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

[nextercism] Implement workspace command #448

Merged
merged 1 commit into from
Aug 17, 2017

Conversation

kytrinyx
Copy link
Member

This adds a simple workspace command that outputs the absolute path to the Exercism workspace. This can be composed with the cd command on Mac, Linux, or in Windows Powershell in order to take the user straight there.

cd $(exercism workspace)

Closes #413

@kytrinyx kytrinyx requested a review from a team August 16, 2017 18:57
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

The command implementation is good - just a few minor suggestions on the command descriptions.

cmd/workspace.go Outdated
@@ -15,16 +16,18 @@ var workspaceCmd = &cobra.Command{

This command can be combined with shell commands to take you there.

For example, on Linux or MacOS you can run:
For example you can run:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the descriptions could be reworded. I recommend something like

Short: Workspace outputs the path to your configured Exercism workspace.
Long: Workspace outputs a normalized version of the configured Exercism workspace path that can be used for scripting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the repetition of workspace sounds right, and I'd change outputs to prints out, so maybe Print out the path to your Exercism workspace.?

Copy link
Contributor

Choose a reason for hiding this comment

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

@robphoenix your version reads better.

@kytrinyx kytrinyx force-pushed the nextercism-workspace-cmd branch from 30cd07e to 9f17f3b Compare August 17, 2017 15:03
@kytrinyx
Copy link
Member Author

I've given some new wording a go. Would you take one last look at this to see if it reads nicely?

Copy link
Contributor

@robphoenix robphoenix left a comment

Choose a reason for hiding this comment

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

One minor nitpick otherwise 👍

cmd/workspace.go Outdated
On Windows, the equivalent command is:

TODO ask @exercism/windows for help
On Windows, this command works in Powershell, however you would need to
Copy link
Contributor

Choose a reason for hiding this comment

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

This reference to this command may be confusing as to whether it is referring to the workspace command or the cd ... command above. Maybe On Windows, this will work only with Powershell, however ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, yeah that's better.

@kytrinyx kytrinyx force-pushed the nextercism-workspace-cmd branch from 9f17f3b to 1062506 Compare August 17, 2017 17:04
@kytrinyx
Copy link
Member Author

I'll merge this when it goes green. I imagine we'll want to nitpick on all the wording for the entire CLI once we see it in action.

@kytrinyx kytrinyx merged commit 03435ce into nextercism Aug 17, 2017
@kytrinyx kytrinyx deleted the nextercism-workspace-cmd branch August 17, 2017 17:41
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.

3 participants