-
-
Notifications
You must be signed in to change notification settings - Fork 368
Move .solution.json to hidden subdirectory #630
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
Changes from 1 commit
8f5493f
cef955f
60b57f1
34c4ec6
c76ab75
964e5c4
bea3009
4ad8121
642f601
858abd2
c363a1f
eaca5f3
5a641a5
9a2c772
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,7 +84,7 @@ func TestDownload(t *testing.T) { | |
| }, | ||
| { | ||
| desc: "It creates the .solution.json file.", | ||
| path: filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", ".solution.json"), | ||
| path: filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", "./.exercism/.solution.json"), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't hard-code slashes here, as on Windows the slashes go the other way. |
||
| contents: `{"track":"bogus-track","exercise":"bogus-exercise","id":"bogus-id","url":"","handle":"alice","is_requester":true,"auto_approve":false}`, | ||
| }, | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,11 @@ import ( | |
| "github.com/exercism/cli/visibility" | ||
| ) | ||
|
|
||
| const ignoreSubdir = ".exercism" | ||
| const solutionFilename = ".solution.json" | ||
|
|
||
| var solutionRelPath = filepath.Join(ignoreSubdir, solutionFilename) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this is only used within NewSolution does it need to be a package level variable?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It gets used in 3 places (including workspace/workspace.go); it seems worth doing to encapsulate the path in one place. Is inlining
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Im speaking about solutionRelPath. I did a quick search and didn’t see other references. If that variable is used three times and we don’t plan on changing it for testing should it be const?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can't be const:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I wasn’t clear, if we make solutionFilename a const with the new path we wouldn’t need the other variables as they are just being used to construct the final solution path I also think it should be exported to denote that it is actually shared or is expected to be.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. It appears that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well then it seems like the best advise I can give is to carry on as you were. Disregarding my coments on this file. I didnt think about the the separator not being known until run time making it unfit to use as a const, my fault. Thanks for vetting and for your time.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem. I appreciate your feedback. |
||
|
|
||
| // Solution contains metadata about a user's solution. | ||
| type Solution struct { | ||
| Track string `json:"track"` | ||
|
|
@@ -29,7 +32,7 @@ type Solution struct { | |
|
|
||
| // NewSolution reads solution metadata from a file in the given directory. | ||
| func NewSolution(dir string) (*Solution, error) { | ||
| path := filepath.Join(dir, solutionFilename) | ||
| path := filepath.Join(dir, solutionRelPath) | ||
| b, err := ioutil.ReadFile(path) | ||
| if err != nil { | ||
| return &Solution{}, err | ||
|
|
@@ -67,11 +70,13 @@ func (s *Solution) Write(dir string) error { | |
| return err | ||
| } | ||
|
|
||
| path := filepath.Join(dir, solutionFilename) | ||
| path := filepath.Join(dir, ignoreSubdir) | ||
| _ = os.Mkdir(path, os.FileMode(0755)) | ||
|
|
||
| // Hack because ioutil.WriteFile fails on hidden files | ||
| visibility.ShowFile(path) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we are calling ShowFile on a directory then down below we are returning the result of visibility.HideFile on the full path to the solution file. At this point I wonder if it is needed since it is now the directory that is a dot file. |
||
|
|
||
| path = filepath.Join(dir, solutionRelPath) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should now be |
||
| if err := ioutil.WriteFile(path, b, os.FileMode(0600)); err != nil { | ||
| return err | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not being used anywhere. Is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, not sure how that slipped through.