Add Download feature #143
Add Download feature #143
Conversation
| @@ -1,5 +1,3 @@ | |||
| #!/bin/bash | |||
|
|
|||
| set -e | |||
|
|
|||
There was a problem hiding this comment.
I suppose we could delete this, since the script only has a single command in it, but I would rather do that in a separate commit, since it doesn't have anything to do with the download command.
There was a problem hiding this comment.
I'm curious: why did you change the file modes on the files below?
I'm open to discussing it, but if we do that it should not be mixed into the commit that adds the new feature.
There was a problem hiding this comment.
What I noticed was, when an compilation error occurs on `go build', it would shutdown the shell rather than shutting down build. Removing the exit flag solved that problem. I will put set -e back in for now.
There was a problem hiding this comment.
Oh, interesting. I'll experiment a bit. It sounds like we will probably want to delete it.
|
Thanks! One more thing -- would you get rid of the merge commit (1b4e336), please? It might just disappear on your own if you rebase against the upstream master. If you don't have an upstream set, you can use: Then If that doesn't work, there are a number of things that you could do. I would probably try something like this: Then when it opens the editor with the list of commits in it, delete the merge commit, save the file, and it will reapply the second patch without the first patch. If that gets complicated let me know and I'll help sort it out. |
| log.Fatal(err) | ||
| } | ||
|
|
||
| for k := range submission.Problem { |
There was a problem hiding this comment.
This can be simplified a bit by getting both the key and the value:
for k, v := range submission.Problem {
// ...
}For clarity, perhaps we can call the local variables name and contents or something like that. Especially now that the map will be called submission.ProblemFiles, name and contents should be clear enough (filename and fileContents would probably be too much of an echo).
|
It looks like this fixes most of the fixes listed above. I also got rid of the merge commit. Thank you so much for taking your time to point these things out! If there's more places to fix, I'll get right on it. |
| Name string `json:"name"` | ||
| Username string `json:"username"` | ||
| Problem map[string]string `json:"problem_files"` | ||
| Code map[string]string `json:"solution_files"` |
There was a problem hiding this comment.
Can these be named ProblemFiles and SolutionFiles to match the new names from the API?
Since they're collections of things, the code becomes much easier to read if they're named with plurals.
|
Thank you! This is looking great. The only two things that I noticed are
Lastly, once it's all cleaned up, would you mind squashing the commits? |
Revert version string Fix documentation for download Return -e flag for bash Fix to use filepath.Join Replace switch..case for argument check Fix download usage msg and empty line in error handling Replaced tab to space Fixed upcoming change for json (problem, solution) Renamed UserName to Username for consistency Fixed use of change "Username" Shorted for loop to deal with key and value Named key and value variable Fixed key, value error Change to ProblemFiles and SolutionFiles Changed bin/* files back to permission 755 Post pull request fixes
|
As it turns out (at least from windows point-of-view) git sets -x on file permission when checkout. It make sense since bash builds cannot be run on Windows OS. I manually added I also squashed all commits after 6d823d8. |
|
I squashed both commits into a single commit, since the entire change is "add download feature". I also ran The merge is here: 4c5fd74 Thank you so much for taking the time to work on this! |
Added a submission "download" feature for #131. It saves problems and solutions using
/submissions/:key. at exercism_home/solutions/:user/:language/:slug/:key/