Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] New CLI for v2 redesign of Exercism #410

Merged
merged 0 commits into from
Jul 13, 2018
Merged

[WIP] New CLI for v2 redesign of Exercism #410

merged 0 commits into from
Jul 13, 2018

Conversation

kytrinyx
Copy link
Member

@kytrinyx kytrinyx commented Aug 4, 2017

🚧 🚧 🚧 PLEASE DO NOT MERGE 🚧 🚧 🚧

This forms the basis for a complete redesign of the CLI. I'm working off of the proposal in exercism/discussions#129 (comment), though there will likely be small differences as I get into the nitty-gritty details and discover that I hadn't thought certain edge cases through.

I will open issues for each of the sub-tasks, each of which will be closed by submitting a pull request to the nextercism branch rather than to master. After all the functionality for launch is ready, we will cut an alpha release, which will not affect the upgrade command for people using the current client.

I've left in a lot of the logic that we already have, but need to rewrite pretty much everything to do with downloading exercises and submitting solutions, as the new site deals with this very differently. I will also be moving all the strings that we use to communicate with users into a settings configuration file, which the CLI will update on a regular basis by calling an endpoint on the website.

This pull request does a few things:

  • updates the README and moves development instructions into a Contributing guide
  • creates the basic structure of the app using the github.com/spf13/cobra library
  • stubs in empty commands for all the commands that I want in the initial release
  • adds basic user and api configuration
  • updates the api and cli packages to work with the new configuration objects
  • updates all the godeps

It leaves in all the basic api client implementation stuff, as well as the functionality that is driving the debug, upgrade, and open commands. The commands themselves will have to be reimplemented within Cobra, but that should be quite straight forward.

New configuration implementation

The new config package in this PR is worth looking at. I've used github.com/spf13/viper as the basis of the functionality, which means that we can bind the existing configs to command-line flags where that makes sense.

We've had a number of discussions about where configuration files should go, most recently in #400

In this new implementation of the CLI, we're operating with the concept of the config home directory, which can be accessed via config.Dir(). This is implemented in dir.go. This directory will contain several configuration files: user.json, cli.json, and api.json.

The config home directory can be configured via the EXERCISM_CONFIG_HOME environment variable. If that is not set, the CLI will use the XDG_CONFIG_HOME variable. If that isn't set either, it will use ~/.config/exercism on Linux and MacOS, and %APPDATA% on Windows.

I'm going to need help from the @exercism/windows folks testing the Windows part, as I don't have access to a Windows machine.

The core configuration type in config.go can be extended by embedding it into other configs. There is an example of how this can be implemented in the config_test.go file, which creates and tests a FakeConfig type.

The API configuration consists of the base url for the Exercism API, and the endpoints to use for various API calls. These will be defined and written by default, but will allow us to deprecate endpoints, to more easily test against local or test environments. The tests are in api_config_test.go.

The user configuration is where we will store user-specific settings such as their API token, their Exercism workspace directory, and their preferences for auto-updates or overwriting files, etc. At the moment I've only implemented token and workspace.

There are OS-specific tests, and I'll need help from @exercism/windows implementing the windows ones.

I've not yet written the CLI config, which will contain all of the strings used to communicate with the user. Error messages, prompts, feedback, etc.

TODO

Closes exercism/discussions#129
Closes #400

@kytrinyx kytrinyx self-assigned this Aug 4, 2017
@kytrinyx kytrinyx requested a review from a team August 4, 2017 18:33
@rpottsoh
Copy link
Member

rpottsoh commented Aug 4, 2017

I'm going to need help from the exercism/windows folks testing the Windows part, as I don't have access to a Windows machine.

Happy to help however I can. I'm also very interested to stay up on how configure develops so that I may update the Windows Installer accordingly.

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 4, 2017

I'm also very interested to stay up on how configure develops so that I may update the Windows Installer accordingly.

That's a great call!

Now that I think about it, I think we need to wait for the PR with the configure command to test the windows stuff, unless one of the windows folks has Go installed locally. That way we'll have something that easily exposes the workspace and config dir functionality.

@robphoenix
Copy link
Contributor

robphoenix commented Aug 4, 2017

I'll be able to give this some proper attention after the weekend, and give it a try on my Windows laptop.

p.s. I have Go 🚀

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 4, 2017

I'll be able to give this some proper attention after the weekend, and give it a try on my Windows laptop.

Awesome! By then I'll have a bunch of things ready for other eyes, so that's perfect!

@rpottsoh
Copy link
Member

rpottsoh commented Aug 4, 2017

@kytrinyx I also have Go installed locally as well and have been able to compile the CLI before. I was able to build what became 2.4.1 prior to that release. You have some options. 👍

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 4, 2017

@rpottsoh perfect!

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 4, 2017

Godeps is incredibly frustrating to work with. If someone can help me figure out what command to run to get the dependencies set up for Travis, that would be 🌟

@Tonkpils
Copy link
Contributor

Tonkpils commented Aug 4, 2017

@kytrinyx what version of Godep and Go are you using? When you're ready to write the deps to the vendor folder it should just be godep save then Travis should be able to pick them up from vendor when doing go install. Are you running into any particular issue? Id be happy to help

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 4, 2017

@Tonkpils I have the most recent version of Godep and I'm running Go 1.8.3

I ran godep save and it didn't save anything. Then I ran godep save ./... and it saved a bunch of stuff, but Travis is complaining about missing dependencies so either Travis isn't installing them, or Godep isn't saving them.

api/api.go:7:2: cannot find package "golang.org/x/net/html/charset" in any of:
	/home/travis/gopath/src/github.com/exercism/cli/vendor/golang.org/x/net/html/charset (vendor tree)
	/home/travis/.gimme/versions/go1.7.linux.amd64/src/golang.org/x/net/html/charset (from $GOROOT)
	/home/travis/gopath/src/golang.org/x/net/html/charset (from $GOPATH)
vendor/github.com/spf13/afero/util.go:29:2: cannot find package "golang.org/x/text/transform" in any of:
	/home/travis/gopath/src/github.com/exercism/cli/vendor/golang.org/x/text/transform (vendor tree)
	/home/travis/.gimme/versions/go1.7.linux.amd64/src/golang.org/x/text/transform (from $GOROOT)
	/home/travis/gopath/src/golang.org/x/text/transform (from $GOPATH)

They're both in the Godeps.json:

@robphoenix
Copy link
Contributor

robphoenix commented Aug 4, 2017

Is Godeps set in stone? I feel like maybe as good Go community members we should maybe be using dep. This being re-written is a good opportunity to change? I've used it on small projects and it's pretty awesome, though saying that I've not used any other dependency management tool, so...
Just a thought anyway. (A thought I've had about configlet also)

@Tonkpils
Copy link
Contributor

Tonkpils commented Aug 4, 2017

It looks like the deps are not getting written to the vendor folder. When you run godep save does it add a change set in git for the vendored packages? I can take a look at this once I'm in front of a computer. As far as using dep, it does look like a good tool moving forward as it's part of the golang org, however I'm not familiar with it or how it works. I can give my two cents once I read a bit more about it!

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 4, 2017

Godep is definitely not set in stone! I only used it because we picked it last time.

Now that the community has made progress I'd love to do whatever is the right(est) thing... I just didn't know what that was :-)

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 4, 2017

It looks like the deps are not getting written to the vendor folder. When you run godep save does it add a change set in git for the vendored packages?

Yepp. Weird!

As far as using dep, it does look like a good tool moving forward as it's part of the golang org, however I'm not familiar with it or how it works. I can give my two cents once I read a bit more about it!

I'll take a look at it today, too, once I finish opening issues for the various commands.

@Tonkpils
Copy link
Contributor

Tonkpils commented Aug 5, 2017

@robphoenix @kytrinyx I've switched us over to dep and tests are passing on travis so that's encouraging. I feel if we're going to do this now is the time since we'll be actively developing against this and we can find which deps work against the new code.

#420

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 5, 2017

I feel if we're going to do this now is the time since we'll be actively developing against this and we can find which deps work against the new code.

That was quick. Thanks so much for digging into that! I'll implement the same thing into this branch.

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 5, 2017

Oh! Hah. Your PR is to this branch. Even better ❤️ ❤️ ❤️

@robphoenix
Copy link
Contributor

Thanks for the switch @Tonkpils feels like the right thing to do.

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 5, 2017

feels like the right thing to do.

Agreed, this feels much cleaner.

cmd/cmd.go Outdated
return strings.Contains(path, "README")
// Bail handles exitable errors in commands.
// TODO: figure out what goes here.
func Bail(err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth including the if err != nil bit in this function as well, maybe make it a CheckError function instead.
Then this

if err != nil {
    Bail(err)
}

could become this

CheckError(err)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would be nice. Maybe BailOnError(err)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's better.
I'm happy to refactor the code and open a PR to this branch. I know you're deep in design and build.

@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 9, 2017

I'm happy to refactor the code and open a PR to this branch.

That would be immensely helpful, thank you ❤️

@kytrinyx kytrinyx changed the title [WIP] Set up the structure for the nextercism CLI [WIP] New CLI for v2 redesign of Exercism Oct 1, 2017
@kytrinyx kytrinyx force-pushed the nextercism branch 2 times, most recently from 8ec044b to 7e40350 Compare July 13, 2018 13:53
@kytrinyx kytrinyx merged commit 7e40350 into master Jul 13, 2018
@kytrinyx kytrinyx deleted the nextercism branch July 14, 2018 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support the XDG Base Directory Specification CLI feature set
4 participants