improve open cmd #788

Smarticles101 · 2019-01-07T17:04:17Z

This pull request aims to improve the open command.

I start by solving #460, while diving into the open command, I found that it could be improved in a few various ways.

cmd/open.go

Smarticles101

I think this is ready for review now, I've made the changes I feel best for consistency with the download command. I'll add tests after it's reviewed to make sure everything is how we want.

Smarticles101 · 2019-01-08T16:04:48Z

cmd/open.go

+				continue
+			}
+
+			if meta.Team != teamID {


Initially, I tried teamID != "" && meta.Team != teamID, but the current boolean statement is more in line with how the api handles team exercises.

Smarticles101 · 2019-01-08T16:05:33Z

cmd/open_test.go

@@ -0,0 +1 @@
+package cmd


I'd like to add tests after this is reviewed, to check that functionality is how we want it

Smarticles101 · 2019-01-08T16:06:27Z

workspace/workspace.go

@@ -57,6 +57,16 @@ func (ws Workspace) PotentialExercises() ([]Exercise, error) {
 			continue
 		}

+		if topInfo.Name() == "teams" {


I needed to add support for teams to PotentialExercises as before this change, it doesn't find them

Would you mind extracting this change into a separate PR? (We should probably include tests for the change).

kytrinyx · 2019-02-20T00:13:08Z

I completely failed to keep up with my Exercism notifications, and I'm inexcusably late to the party. To add to this, my laptop crashed and is now in for repair, and as of today I have a loaner that I've not quite gotten set up to be usable. So. My apologies (first) and (second)... my goal is to get through all the open PRs in the CLI track over the course of the next week. So sorry to leave you hanging!

Smarticles101 · 2019-02-20T15:26:34Z

I completely failed to keep up with my Exercism notifications, and I'm inexcusably late to the party. To add to this, my laptop crashed and is now in for repair, and as of today I have a loaner that I've not quite gotten set up to be usable. So. My apologies (first) and (second)... my goal is to get through all the open PRs in the CLI track over the course of the next week. So sorry to leave you hanging!

Don't worry about it too much! Things happen and I'm pretty sure most people here are guilty of ignoring notifications for a long time. Completely understandable

kytrinyx · 2019-02-26T21:29:54Z

Hey @Smarticles101 -- overall it looks like what you have here is much better.

In order to help me review this properly, would you summarize exactly what changed, and how this will change the usage of the command?

Smarticles101 · 2019-02-26T23:49:57Z

@kytrinyx
The old open command worked in a way similar to this:

exercism open ./path/to/exercise/folder

The new command, rather than using an exercise path, follows a style similar to the other commands. Now instead of passing an exercise directory, you pass a slug.

exercism open --exercise slug

I also added support for --team, --track, and --remote from #460 .

kytrinyx · 2019-02-27T00:39:41Z

Thank you, that's really helpful!

In order to not break people's work flow we'll need to support the original usage as well, though... at least until we do a major version bump.

Smarticles101 · 2019-02-27T02:36:46Z

@kytrinyx That sounds good, I can add that back in when I have free time tomorrow

Smarticles101 · 2019-02-28T14:32:03Z

@kytrinyx original usage is supported now :)

Smarticles101 · 2019-02-28T14:51:28Z

No idea why the build is failing, I haven't changed anything that should make it fail, and I've rebased against master and compiled it locally and it seems to work?

kytrinyx · 2019-02-28T15:13:34Z

original usage is supported now :)

w000p!!!!

No idea why the build is failing

I'll take a look.

kytrinyx · 2019-02-28T15:24:02Z

cmd/open.go

+				return err
+			}
+
+			if meta.Exercise != exerciseID {


This needs to be meta.ExerciseSlug to pass the tests (see #815)

kytrinyx

This is looking really good. I have a few suggestions, but overall I think this is doing the right thing!

kytrinyx · 2019-02-28T15:25:30Z

cmd/open.go

+	usrCfg := cfg.UserViperConfig
+	trackID, _ := flags.GetString("track")
+	exerciseID, _ := flags.GetString("exercise")
+	teamID, _ := flags.GetString("team")


I know error checking seems tedious, but I'd rather we did it for completion. If it gets really bad we can extract it into a validator type, but for now let's just do it inline.

kytrinyx · 2019-02-28T15:26:46Z

cmd/open.go

+	var url string
+	usrCfg := cfg.UserViperConfig
+	trackID, _ := flags.GetString("track")
+	exerciseID, _ := flags.GetString("exercise")


Let's name this exerciseSlug for consistency with the rest of the codebase (we don't currently call it exerciseID anywhere).

kytrinyx · 2019-02-28T15:27:24Z

cmd/open.go

+	usrCfg := cfg.UserViperConfig
+	trackID, _ := flags.GetString("track")
+	exerciseID, _ := flags.GetString("exercise")
+	teamID, _ := flags.GetString("team")


Cat we go with just team as the variable name? We don't call this teamID anywhere else.

kytrinyx · 2019-02-28T15:27:57Z

cmd/open.go

 		metadata, err := workspace.NewExerciseMetadata(args[0])
 		if err != nil {
 			return err
 		}
 		browser.Open(metadata.URL)
 		return nil
-	},
+		//return fmt.Errorf("Must provide an `--exercise`")


Let's ✂️ the commented out stuff.

kytrinyx · 2019-02-28T15:30:33Z

cmd/open.go

+	teamID, _ := flags.GetString("team")
+
+	if exerciseID == "" {
+		// if no --exercise is given, use original functionality
 		metadata, err := workspace.NewExerciseMetadata(args[0])


How about extracting args[0] into a variable path up at the start where we're also extracting from flags?

var path string if args.length > 0 { path = args[0] }

That way down here we can say if path != "" {

If we want to be really explicit, we could also have a validation on path != "" and exerciseSlug != "" and explain that you need one or the other.

kytrinyx · 2019-02-28T15:30:56Z

cmd/open.go

+		//return fmt.Errorf("Must provide an `--exercise`")
+	}
+
+	if remote, _ := flags.GetBool("remote"); remote {


Let's extract the remote flag up top as well, and check the error.

kytrinyx · 2019-02-28T15:32:28Z

cmd/open.go

+		var payload openPayload
+		if err := json.NewDecoder(res.Body).Decode(&payload); err != nil {
+			return fmt.Errorf("unable to parse API response - %s", err)
+		}


Would it make sense to use the api error decoder helper?

cli/cmd/cmd.go

Line 78 in c3525cf

func decodedAPIError(resp *http.Response) error {

I need to decode the json anyways for the solution url. The helper only returns the error. Would it be beneficial to introduce a helper that decodes the entire api response?

Hm. Maybe, but let's hold off on that for now.

kytrinyx · 2019-02-28T15:33:10Z

cmd/open.go

+
+		if res.StatusCode != http.StatusOK {
+			switch payload.Error.Type {
+			case "track_ambiguous":


I wonder if we can make the error decoder helper smarter and add this logic into it.

cli/cmd/cmd.go

Line 78 in c3525cf

func decodedAPIError(resp *http.Response) error {

(If so, let's do that in a separate PR and get that into master quickly so we can use it elsewhere).

kytrinyx · 2019-02-28T15:33:43Z

cmd/open.go

+			}
+		}
+
+		url = payload.Solution.URL


Let's open the URL and return here so that we can out-dent the else.

kytrinyx · 2019-02-28T15:35:58Z

workspace/workspace.go

@@ -57,6 +57,16 @@ func (ws Workspace) PotentialExercises() ([]Exercise, error) {
 			continue
 		}

+		if topInfo.Name() == "teams" {


Would you mind extracting this change into a separate PR? (We should probably include tests for the change).

Smarticles101 · 2019-03-01T00:55:09Z

I made a mess of my git tree today and ended up having to cherry-pick my commits out.

I updated it to use ExerciseSlug now so the build passes. I'll see when I have time to look into your other comments. I should get some time early next week

kytrinyx · 2019-03-03T16:54:17Z

@Smarticles101 just a quick note to say that I think the next step here is to rebase onto master before I do another review. If I'm wrong about that please let me know :-)

now you just give the exercise name instead of path

kytrinyx · 2019-03-15T15:49:00Z

@Smarticles101 Is this ready for another round of review?

kytrinyx

The current logic around path or slug is a bit confusing, so I've suggested an improvement there.

We're also going to need to fix the long description for the command.

kytrinyx · 2019-03-15T15:54:22Z

cmd/open.go

+			return fmt.Errorf("must provide an --exercise slug or an exercise path")
+		}
+		// if no --exercise is given, use original functionality
+		metadata, err := workspace.NewExerciseMetadata(path)


I think that this would be easier to understand if we did something like this:

if exerciseSlug == "" && path == "" { return fmt.Errorf... } if path != "" { // get metadata and open the url } // etc

Smarticles101 · 2020-10-22T01:19:46Z

I know it's been nearly two years, but I thought I would get this finished up. I've addressed the last changes requested. If everything looks good I will write some tests.

Smarticles101 · 2020-10-22T03:10:55Z

@ekingery if you'd like to take a look at this and let me know if you see anything that's off, I'd appreciate it :)

ekingery · 2020-10-23T01:52:48Z

@ekingery if you'd like to take a look at this and let me know if you see anything that's off, I'd appreciate it :)

Thanks @Smarticles101! We're fully focused on getting configlet updated for V3 right now, but this change looks like something we should be able to get merged as soon as we can bring some focus back to the CLI and get a new version out. It looks like the build is broken at the moment, any ideas on that?

Smarticles101 · 2020-10-23T03:17:00Z

@ekingery yes, like in #957, it fails on older versions of go presumably due to outdated modules. It builds correctly on the latest version

ErikSchierboom · 2024-05-02T10:15:55Z

This is quite an old PR and has some merge conflicts. @Smarticles101 do you still want to work on this?

Smarticles101 commented Jan 7, 2019

View reviewed changes

cmd/open.go Show resolved Hide resolved

Smarticles101 force-pushed the development branch 2 times, most recently from 1c1c997 to d0c429f Compare January 8, 2019 15:39

Smarticles101 mentioned this pull request Jan 8, 2019

Odd teams api errors exercism/exercism#4587

Closed

Smarticles101 commented Jan 8, 2019

View reviewed changes

Smarticles101 changed the title ~~[WIP] improve open cmd~~ improve open cmd Jan 10, 2019

kytrinyx reviewed Feb 28, 2019

View reviewed changes

Smarticles101 force-pushed the development branch 3 times, most recently from 0ef90c5 to 4f9bb94 Compare February 28, 2019 17:42

Smarticles101 mentioned this pull request Mar 1, 2019

Add teams support to workspace.PotentialExercises #827

Merged

Smarticles101 added 6 commits March 5, 2019 08:33

implement --remote flag for open exercism#460

e42a157

open command is aware of the exercism directory

4a7c94f

now you just give the exercise name instead of path

open uses --exercise

24d789a

open: support for --team

d3d82c4

open: original functionality works

49c072d

open: change meta.Exercise to meta.ExerciseSlug

a80741c

Smarticles101 force-pushed the development branch from 4f9bb94 to a80741c Compare March 5, 2019 14:34

open: implement suggestions

d0d0655

kytrinyx requested changes Mar 15, 2019

View reviewed changes

Smarticles101 force-pushed the development branch from fd60095 to d0d0655 Compare October 22, 2020 00:09

Smarticles101 added 2 commits October 21, 2020 18:20

Merge branch 'master' into development

764d72f

open: clean up logic and improve description

76cf131

open: remove teams support. ref exercism#956

ce6efb2

Base automatically changed from master to main January 28, 2021 22:32

improve open cmd #788

Are you sure you want to change the base?

improve open cmd #788

Conversation

Smarticles101 commented Jan 7, 2019

Smarticles101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kytrinyx commented Feb 20, 2019

Smarticles101 commented Feb 20, 2019

kytrinyx commented Feb 26, 2019

Smarticles101 commented Feb 26, 2019 • edited

kytrinyx commented Feb 27, 2019

Smarticles101 commented Feb 27, 2019

Smarticles101 commented Feb 28, 2019

Smarticles101 commented Feb 28, 2019

kytrinyx commented Feb 28, 2019

Choose a reason for hiding this comment

kytrinyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Smarticles101 commented Mar 1, 2019

kytrinyx commented Mar 3, 2019

kytrinyx commented Mar 15, 2019

kytrinyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Smarticles101 commented Oct 22, 2020

Smarticles101 commented Oct 22, 2020

ekingery commented Oct 23, 2020

Smarticles101 commented Oct 23, 2020

ErikSchierboom commented May 2, 2024

Smarticles101 commented Feb 26, 2019 •

edited