Skip to content

Update submit error message when submitting a directory#724

Merged
nywilken merged 5 commits intoexercism:masterfrom
sfairchild:development
Aug 31, 2018
Merged

Update submit error message when submitting a directory#724
nywilken merged 5 commits intoexercism:masterfrom
sfairchild:development

Conversation

@sfairchild
Copy link
Copy Markdown
Contributor

This it updating the error message when trying to submit a directory to help new students.
Issue: exercism/exercism#4309

@sfairchild sfairchild changed the title Development Update submit error message when submitting a directory Aug 29, 2018
Copy link
Copy Markdown
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.

@sfairchild thank you for your contribution and for improving the CLI. I have a few minor changes, but otherwise this looks good.

Comment thread cmd/submit.go Outdated
@nywilken
Copy link
Copy Markdown
Contributor

@sfairchild \please update the formatting so that it is consistent with the message format being used for the download message error

cli/cmd/submit.go

Lines 138 to 144 in e9877e2

The solution you are submitting is not connected to your account.
Please re-download the exercise to make sure it has the data it needs.
%s download --exercise=%s --track=%s
`
return fmt.Errorf(msg, BinaryName, solution.Exercise, solution.Track)

@sfairchild
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback. I updated and pushed the changes

Comment thread cmd/submit.go Outdated

%s

Please provide the path to the file you wish to submit, e.g. : %s submit FILENAME
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One more minor nitpick. We generally put the example command on a separate line to avoid it being pushed off the terminal window (longer strings) and to make is easier for copying/pasting.

Can we update the message so that it reads and looks like the following.

    You are submitting a directory, which is not currently supported.

        /home/wilken/Development/exercism/vimscript

    Please change into the directory and provide the path to the file(s) you wish to submit

        exercism submit FILE1 FILE2

Note: I am making the assumption that if a user is trying to submit a directory they probably want to submit multiple files; hence the reason for calling out FILE1 FILE2. If that is not the case feel free to just keep FILENAME.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The use case for this was a new student trying to submit hello_world for the very first time and not knowing how to fix their issue with just the line

You are submitting a directory, which is not currently supported.

Listing the multiple files might be helpful for letting users know that you can submit more than 1 file. I didn't even know this was an option until I dug into the source. My fear is listing multiple files will be confusing for someone who if very new. I am happy to make the change based on what everyone else thinks. @kytrinyx


    Please change into the directory and provide the path to the file(s) you wish to submit

        exercism submit FILE1 FILE2

vs


    Please change into the directory and provide the path to the file(s) you wish to submit

        exercism submit FILE

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let’s drop the multiple files message. I agree that it can be confusing. Which is why I called out the note.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is a common convention to put optional arguments (named and positional) into square brackets, also using ellipsis to signal "or more", so that line should IMHO read exercism submit FILE1 [FILE2 ...]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But I think, a single file example is sufficient if we extend the usage (exercism help submit) as described below and remind in the error that you can get more info there.

  1. be more explicit about the fact that at least one file is necessary
  2. be more explicit that one or more files can be specified

The current help output only states in the description that a list of files is needed, the actual usage does not mention it, but flags only as optional (square brackets).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with @NobbZ that the documentation for submit could be improved. Maybe a seperate issue could be opened to update that.

➜  ~ exercism version
exercism version 3.0.9
➜  ~ exercism submit -h
Submit your solution to an Exercism exercise.

	Call the command with the list of files you want to submit.

Usage:
   submit [flags]

Aliases:
  submit, s

Flags:
  -h, --help   help for submit

Global Flags:
      --timeout int   override the default HTTP timeout (seconds)
  -v, --verbose       verbose output

For now I updated the error message to

    Please change into the directory and provide the path to the file(s) you wish to submit

        exercism submit FILENAME

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sfairchild seeing this now. Thanks for the update. I’ll open an issue for the help command, if you haven’t already, and please feel free to add any additional requirements for the usage details.

Comment thread cmd/submit.go

`
return fmt.Errorf(msg, arg)
return fmt.Errorf(msg, arg, BinaryName)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@nywilken
Copy link
Copy Markdown
Contributor

so that line should IMHO read exercism submit FILE1 [FILE2 ...]

@NobbZ thanks for pointing this out. That is the right way and we should definitely add that output to our usage details.

extend the usage ( exercism help submit ) ... and remind in the error that you can get more info there.

Let’s open a separate PR for this one. If you come across other info that should be part of the usage, please let us know. Ty

@nywilken nywilken merged commit cdb407e into exercism:master Aug 31, 2018
@sfairchild
Copy link
Copy Markdown
Contributor Author

@nywilken fyi, issue #728 opened

@nywilken
Copy link
Copy Markdown
Contributor

@sfairchild I got caught up working on issue #719 and forgot to open. Ty.

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