Skip to content

add check if the submitted filename is a directory#302

Merged
kytrinyx merged 1 commit intoexercism:masterfrom
alebaffa:submit-should-warn-users-that-try-to-check-in-a-directory
Mar 25, 2016
Merged

add check if the submitted filename is a directory#302
kytrinyx merged 1 commit intoexercism:masterfrom
alebaffa:submit-should-warn-users-that-try-to-check-in-a-directory

Conversation

@alebaffa
Copy link
Contributor

I tried to add some tests, but I didn't know if to add a new one into_ paths/paths_test.go_ (so basically to testing the function IsDir()), or create a new cmd/submit_test.go.
Let me know if you need a test.

cmd/submit.go Outdated
}

if paths.IsDir(filename) {
log.Fatal("You cannot submit a directory. Submit only the solution file.")
Copy link
Member

Choose a reason for hiding this comment

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

You can submit more than one file.

How about:

Please specify each file that should be submitted, e.g. `exercism submit file1 file2 file3`.

@kytrinyx
Copy link
Member

No, don't worry about a test. Eventually we'll be adding tests for the commands, but it's going to take some thought to get it right.

@alebaffa
Copy link
Contributor Author

Just for my curiosity: what are the considerations you are making for the tests for the commands? (don't know if it's the right place to ask this .. :P )

@Tonkpils
Copy link
Contributor

what are the considerations you are making for the tests for the commands?

We are using a 3rd party library to handle the CLI interface. In order to test the commands we have to mock the argument passed into the command functions which would require a change of signature of all the functions to pass an interface of our own. However, the CLI package takes a specific struct which doesn't allow for this kind of change.

Unless that's changed in the CLI package, that's the one thing stopping us from writing tests around the commands.

@kytrinyx
Copy link
Member

This looks good to me. Would you mind squashing the commits, please?

update the error message considering that the user can update more than one file.
@alebaffa
Copy link
Contributor Author

Voila :)

@kytrinyx
Copy link
Member

Lovely, thank you!

@kytrinyx kytrinyx merged commit 8086bf6 into exercism:master Mar 25, 2016
@mhelmetag
Copy link

Fixes #300

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.

4 participants