Move .solution.json to hidden subdirectory by jdsutherland · Pull Request #630 · exercism/cli

jdsutherland · 2018-07-14T06:34:14Z

Closes #607
Closes #557

jdsutherland · 2018-07-14T06:44:36Z

It seems the AppVeyor error is the same as #557. Is the intended solution to rename .solution.json, (i.e., .exercism/solution.json)?

kytrinyx · 2018-07-14T10:49:40Z

s the intended solution to rename .solution.json, (i.e., .exercism/solution.json)?

Yeah we're aiming for .exercism/solution.json

kytrinyx · 2018-07-14T10:51:07Z

 		{
 			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"),


We can't hard-code slashes here, as on Windows the slashes go the other way.

filepath.Join(cmdTest.TmpDir, "bogus-track", "bogus-exercise", ".exercism", "solution.json")

nywilken · 2018-07-14T12:17:36Z

s the intended solution to rename .solution.json, (i.e., .exercism/solution.json)?

To be clear, the path on *nix will be EXERCISEROOT/.exercism/.solution.json and on Windows it should be EXERCISEROOT\.exercism\.solution.json

nywilken

I left a couple of comments. Running test on Windows fail the same way as Appveyor. Interesting enough removing the call to visibility.Hidefile fixes all the tests.

https://github.com/exercism/cli/pull/630/files#diff-9864a541f622543e9591e6d72271deb8R84

nywilken · 2018-07-14T12:31:31Z

 	"github.com/spf13/cobra"
 )

+const hiddenSolutionDir = ".exercism"


This is not being used anywhere. Is it needed?

Oops, not sure how that slipped through.

nywilken · 2018-07-14T12:34:15Z

+const ignoreSubdir = ".exercism"
 const solutionFilename = ".solution.json"

+var solutionRelPath = filepath.Join(ignoreSubdir, solutionFilename)


Seems like this is only used within NewSolution does it need to be a package level variable?

It gets used in 3 places (including workspace/workspace.go); it seems worth doing to encapsulate the path in one place. Is inlining const ignoreSubdir preferred?

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?

It can't be const: [gometalinter] const initializer filepath.Join(ignoreSubdir, solutionFilename) is not a constant (vet) [Error]

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
const SolutionFilename = filepath.Join(“.exercism”,”solution.json”)

I also think it should be exported to denote that it is actually shared or is expected to be.

Gotcha. It appears that filepath.Join cannot be const: [gometalinter] const initializer filepath.Join(".exercism", "solution.json") is not a constant (vet) [Error]

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.

No problem. I appreciate your feedback.

kytrinyx · 2018-07-14T13:23:51Z

To be clear, the path on *nix will be EXERCISEROOT/.exercism/.solution.json and on Windows it should be EXERCISEROOT.exercism.solution.json

I was thinking we don't need the file to be hidden if it's in a hidden directory. So:

*nix: $EXERCISEROOT/.exercism/solution.json
Windows: $EXERCISEROOT\.exercism\solution.jso

kytrinyx · 2018-07-14T13:27:15Z

Interesting enough removing the call to visibility.Hidefile fixes all the tests.

How strongly do we feel that the metadata directory needs to be hidden?

Address review concerns: * fix path string OS compatibility * remove dead var

nywilken · 2018-07-14T14:43:30Z

Something to think about. When we roll this change out it will break already downloaded exercises as the solution json file will be in a new path. Forcing them to redownload to fix.

If that is the case then our code should detect if an existing solution file exists and move it for the user. Thoughts?

NobbZ · 2018-07-14T16:26:38Z

How smooth will the transition be from .solution.json to the directory? Will the CLI fail with an irritating or useless errormessage as the current one does when the file is missing? Or will it migrate when the file is there but not under the directory?

kytrinyx · 2018-07-14T22:55:57Z

When we roll this change out it will break already downloaded exercises as the solution json file will be in a new path. Forcing them to redownload to fix.

Yes, I was hoping to get this sorted before the launch, but it's been an eventful few days and I didn't get to it.

We need to handle detecting the old location and moving the file for the user if it is found.

Adds compatibility for already downloaded exercises

jdsutherland · 2018-07-16T03:55:43Z

We need to handle detecting the old location and moving the file for the user if it is found.

I gave this a shot and think it's in a good state for review.

Interesting enough removing the call to visibility.Hidefile fixes all the tests.

How strongly do we feel that the metadata directory needs to be hidden?

Do we want to remove visibility.Hidefile?

nywilken · 2018-07-16T22:57:49Z

@jdsutherland I’ve been a little caught up. Thanks for updating. I’ll take a look at your latest changes and get back to you by tomorrow.

golint: exported const SolutionFilename should have comment or be unexported [Warning]

Export the joined filepath of the solution metadata file rather than the individual paths

nywilken · 2018-07-22T16:38:54Z

@jdsutherland I haven’t forgotten about this PR. Thanks for keeping it updated. I will tackle this PR with the team PR later today.

nywilken

Do you mean remove all refs of visibility package? Isn't visibility.HideFile(path) needed to hide the directory in Windows?

That was the question I was asking and it seems like it may not be needed now that tests are passing on Windows. We need to thoroughly test with an actual build and file downloads.

nywilken

I had a chance to dive deeper into this review. I left a number of comments and called out a few extra eyes to help answer some open questions. This is just about ready IMO. @FabienTregan if you have a minute or two it would be great to get your eyes on the file visibility code that was removed, as it appears to be no longer needed.

nywilken · 2018-07-24T02:32:30Z

 	assert.Regexp(t, "No files found", err.Error())
 }

+func TestSubmitExerciseWithLegacySolutionMetadataFileAndGetsMigrated(t *testing.T) {


What do you think about calling this method TestLegacySolutionMetadataMigration?

TestLegacySolutionMetadataMigration is more readable. The only potential concern I can think of is that the test isn't just testing migration but that a submission is still successful given a legacy solution file - is it worth indicating that in the name?

nywilken · 2018-07-24T02:37:06Z

+
+	tmpDir, err := ioutil.TempDir("", "legacy-metadata-file")
+	assert.NoError(t, err)
+


These files are not being cleaned up after the tests. A deferred call os.RemoveAll should do the trick.

nywilken · 2018-07-24T02:40:37Z

+		Dir:             tmpDir,
+		UserViperConfig: v,
+	}
+	expectedSolutionPathAfterMigration := filepath.Join(dir, workspace.SolutionMetadataFilepath())


Does it make sense to check that this file doesn’t exist prior to executing submit?

@kytrinyx thoughts, Is it too much?

Yeah, I think it would help clarify intent and ensure completeness.

nywilken · 2018-07-24T02:48:41Z


 // Write stores solution metadata to a file.
-func (s *Solution) Write(dir string) error {
+func (s *Solution) Write(path string) error {


Sorry for the confusion this variable should remain as dir in a previous version you were generating the path, but still using dir to concat filenames.

I think what you have now is good. I just recommend that you keep dir for the exercise directory and use path for the SolutionMetadataPath

nywilken · 2018-07-24T02:53:15Z

+		return err
+	}
+	s.Dir = path
+	return err


return nil is a bit more clear that no error is being returned

nywilken · 2018-07-24T03:14:24Z

+		fmt.Fprintf(os.Stderr, "\nMigrated solution metadata to %s\n", solutionPath)
+	} else {
+		// TODO: decide how to handle case where both legacy and modern metadata files exist
+		fmt.Fprintf(os.Stderr, "\nAttempted to migrate solution metadata to %s but file already exists\n", solutionPath)


I think the new metadata takes precendence which leads into my next comment about workspace,checkSolutionFile. It currently checks for a legacy metdata first, but I think we should check for the new path first and only check for legacy if it doesn’t exist.

Right. The most common path should be checked first.

nywilken · 2018-07-24T03:20:04Z

+	solutionPath := filepath.Join(path, SolutionMetadataFilepath())
+
+	if _, err := os.Lstat(legacySolutionPath); err == nil {
+		return migrateLegacySolutionFile(legacySolutionPath, solutionPath)


If we don’t rename and instead warn the use that a deprecated solution files exists would it make the migration easier. @kytrinyx @NobbZ thoughts on leaving the file or renaming it for the user.

I recommended renaming but I’m not sure how users will feel about the CLI renaming

* rename test * test artifact cleanup * during test, check expected migration path DNE before migration * when checking solution files, check for a modern solution before legacy

jdsutherland · 2018-07-24T07:10:36Z

Do you mean remove all refs of visibility package? Isn't visibility.HideFile(path) needed to hide the directory in Windows?

That was the question I was asking and it seems like it may not be needed now that tests are passing on Windows. We need to thoroughly test with an actual build and file downloads.

If testing on Windows is successful, should the visibility package be deleted as a result of these changes? It wouldn't have any remaining referrers.

kytrinyx · 2018-07-25T05:03:56Z

If testing on Windows is successful, should the visibility package be deleted as a result of these changes? It wouldn't have any remaining referrers.

Yes, if we aren't referring to it, I think we should delete it.

kytrinyx · 2018-07-26T18:55:38Z

 }
+
+// SolutionMetadataFilepath is the path of the solution metadata file relative to the workspace.
+func SolutionMetadataFilepath() string {


I don't understand how this works. Each exercise has its own metadata file, so this is relative to a subdirectory within the workspace.

I mistakenly used 'workspace' when I meant 'exercise' there.

Ok, that makes sense. Thanks!

kytrinyx · 2018-07-26T19:03:07Z

I've started working on a change in #682 that I think would help clarify things here.

With the workspace.Exercise type we could have two methods:

exercise.MetadataFilepath()
exercise.LegacyMetadataFilepath()

I think it would be worth making the change in multiple steps, for clarity.

Rename workspace.Solution to workspace.ExerciseMetadata or workspace.SolutionMetadata for clarity
Once the Detect all exercises in a workspace #682 gets merged, make a single PR that refactors to use workspace.Exercise and rely on exercise.MetadataFilepath() instead of hard-coding .solution.json
Finally, a PR (most of what is in this PR) that changes the default location of MetadataFilepath() and updates the behavior to fall back to looking in LegacyMetadataFilepath() if the default doesn't exist, as well as moving the legacy file to the new path.

Does that make sense?

jdsutherland · 2018-07-28T00:38:21Z

Yeah, that makes sense. I think what you've suggested will improve clarity.

It seems that the terms 'exercise' and 'solution' are used interchangeably in certain places but are defined differently based on https://github.com/exercism/docs/blob/master/about/glossary.md. A few examples:

cli/workspace/workspace.go

Lines 21 to 23 in d48268d

    
           // Workspace represents a user's Exercism workspace. 
        
           // It may contain a user's own exercises, and other people's 
        
           // exercises that they've downloaded to look at or run locally.

cli/cmd/download.go

Lines 28 to 32 in d48268d

    
           You may download an exercise to work on. If you've already 
        
           started working on it, the command will also download your 
        
           latest solution. 
        
           Download other people's solutions by providing the UUID.

If I understand correctly, the metadata is static - it's generated once when the user runs a download cmd and doesn't change when a solution gets submitted. If it doesn't change when a solution is submitted, calling it SolutionMetadata seems counterintuitive. When I first began making these changes (as someone unfamiliar with the codebase and terminology) I expected workspace.Solution to contain stuff related to work the user has done on an exercise but it doesn't actually seem related to anything the user does, rather mostly concerned with metadata that seems more associated with an exercise than a solution. Calling it ExerciseMetadata seems more appropriate.

This confusion might stem from the meaning of 'solution'. Based on the glossary definition, I'd expect a 'solution' to be one or more iterations of an exercise - work done to solve an exercise. Currently, a Solution "has-a" Exercise. Solution appears to represent both the exercise itself (a Solution type is created when an exercise is downloaded for the first time) and an iteration of that exercise. So, the user downloads a 'solution', does work to solve it, and the result is also called a 'solution'. I find this confusing. Maybe I'm not thinking about this clearly.

kytrinyx · 2018-08-01T23:26:59Z

If I understand correctly, the metadata is static - it's generated once when the user runs a download cmd and doesn't change when a solution gets submitted.

I think you're right that ExerciseMetadata makes more sense here. Thanks for helping think this through--getting terminology right is tricky (and so important!).

kytrinyx · 2018-08-26T19:27:49Z

@jdsutherland How are you getting on?

I think we decided to not rename the solution and exercise metadata until after this work is done.

As far as I can tell, the next step would be to open a separate PR that ensures that we never hard-code .solution.json anywhere (that we always rely on the configured constant).

jdsutherland · 2018-08-27T08:22:21Z

@kytrinyx Sorry, I thought this was awaiting changes but it'd been awhile since I checked. I made a new PR (#723).

kytrinyx · 2018-08-28T01:13:07Z

I made a new PR

Awesome, I'll review that now!

Move .solution.json to hidden subdirectory

8f5493f

kytrinyx reviewed Jul 14, 2018

View reviewed changes

nywilken reviewed Jul 14, 2018

View reviewed changes

nywilken mentioned this pull request Jul 14, 2018

Access denied to .solution.json on Windows #557

Closed

Remove dot from solution.json

cef955f

Address review concerns: * fix path string OS compatibility * remove dead var

Add legacy solution handling

60b57f1

Adds compatibility for already downloaded exercises

jdsutherland force-pushed the solution.json-.exercism-dir branch 5 times, most recently from eb77349 to b16fe6e Compare July 16, 2018 03:49

jdsutherland force-pushed the solution.json-.exercism-dir branch 7 times, most recently from 3d1ce31 to 4dd5ab5 Compare July 16, 2018 06:09

Add test for submitting with legacy solution

34c4ec6

jdsutherland force-pushed the solution.json-.exercism-dir branch from 4dd5ab5 to 34c4ec6 Compare July 16, 2018 06:11

jdsutherland force-pushed the solution.json-.exercism-dir branch from e54f2ad to 53ea687 Compare July 21, 2018 08:38

Add comments to exported consts

858abd2

golint: exported const SolutionFilename should have comment or be unexported [Warning]

jdsutherland force-pushed the solution.json-.exercism-dir branch from 53ea687 to 858abd2 Compare July 21, 2018 08:40

nywilken mentioned this pull request Jul 22, 2018

Move assertion for solution metadata out of helper #659

Merged

Refactor solution metadata filepath export

c363a1f

Export the joined filepath of the solution metadata file rather than the individual paths

nywilken reviewed Jul 22, 2018

View reviewed changes

nywilken reviewed Jul 24, 2018

View reviewed changes

Address nywilken's review

eaca5f3

* rename test * test artifact cleanup * during test, check expected migration path DNE before migration * when checking solution files, check for a modern solution before legacy

jdsutherland force-pushed the solution.json-.exercism-dir branch from 6ac00a2 to aa41c17 Compare July 24, 2018 05:25

Merge branch 'master' into solution.json-.exercism-dir

5a641a5

jdsutherland force-pushed the solution.json-.exercism-dir branch from aa41c17 to 5a641a5 Compare July 24, 2018 05:29

Rename err var to avoid shadowing confusion

9a2c772

jdsutherland force-pushed the solution.json-.exercism-dir branch 2 times, most recently from 881a90b to 9a2c772 Compare July 24, 2018 12:25

kytrinyx reviewed Jul 26, 2018

View reviewed changes

kytrinyx added the size/big label Jul 27, 2018

jdsutherland mentioned this pull request Aug 7, 2018

Rename solution to exercise metadata #693

Closed

kytrinyx added the type/bug label Aug 26, 2018

jdsutherland mentioned this pull request Aug 27, 2018

Metadata hidden subdir #723

Merged

kytrinyx closed this in #723 Sep 4, 2018

jdsutherland mentioned this pull request Sep 15, 2018

Rename 'solution metadata' to 'exercise metadata' #736

Merged


		tmpDir, err := ioutil.TempDir("", "legacy-metadata-file")
		assert.NoError(t, err)

Uh oh!

Conversation

jdsutherland commented Jul 14, 2018 • edited by kytrinyx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdsutherland commented Jul 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kytrinyx commented Jul 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nywilken commented Jul 14, 2018

Uh oh!

nywilken left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nywilken Jul 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdsutherland Jul 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kytrinyx commented Jul 14, 2018

Uh oh!

kytrinyx commented Jul 14, 2018

Uh oh!

nywilken commented Jul 14, 2018

Uh oh!

NobbZ commented Jul 14, 2018

Uh oh!

kytrinyx commented Jul 14, 2018

Uh oh!

jdsutherland commented Jul 16, 2018

Uh oh!

nywilken commented Jul 16, 2018

Uh oh!

nywilken commented Jul 22, 2018

Uh oh!

nywilken left a comment

Choose a reason for hiding this comment

Uh oh!

nywilken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdsutherland Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdsutherland commented Jul 14, 2018 •

edited by kytrinyx

Loading

jdsutherland commented Jul 14, 2018 •

edited

Loading

nywilken left a comment •

edited

Loading

nywilken Jul 14, 2018 •

edited

Loading

jdsutherland Jul 14, 2018 •

edited

Loading

jdsutherland Jul 24, 2018 •

edited

Loading

jdsutherland commented Jul 28, 2018 •

edited

Loading