Metadata hidden subdir #723

jdsutherland · 2018-08-27T08:19:25Z

Continuation of #630
Closes #630
Closes #607
Closes #557

jdsutherland · 2018-08-27T10:45:13Z

workspace/exercise_test.go

@@ -17,9 +17,9 @@ func TestHasMetadata(t *testing.T) {
 	exerciseA := Exercise{Root: ws, Track: "bogus-track", Slug: "apple"}
 	exerciseB := Exercise{Root: ws, Track: "bogus-track", Slug: "banana"}

-	err = os.MkdirAll(filepath.Join(exerciseA.Filepath()), os.FileMode(0755))
+	err = os.MkdirAll(filepath.Join(exerciseA.Filepath(), ignoreSubdir), os.FileMode(0755))


Tests having to know to create the .exercism ignore subdirectory seems smelly. I'm not sure how to avoid it though.

How about using https://golang.org/pkg/path/filepath/#Dir on the metadata path? That way if it ever changes, the test won't have to know.

Good call. I think I fixed this up in all the tests that were explicitly knowing about metadata filepaths.

jdsutherland · 2018-08-27T10:45:57Z

workspace/workspace_test.go

 		err := os.MkdirAll(path, os.FileMode(0755))
 		assert.NoError(t, err)

-		if path != a2 {
+		if path != filepath.Join(a2, ignoreSubdir) {


Tests having to know to create the .exercism ignore subdirectory seems smelly. I'm not sure how to avoid it though.

I was thinking os.MkdirAll on the filepath.Dir of the full metadata file filepath -- I'd rather do that than have to have the code know about the various pieces of the metadata file path.

jdsutherland · 2018-08-27T10:49:08Z

workspace/solution.go

+	return nil
+}
+
+func migrateLegacySolutionFile(legacyMetadataPath string, metadataPath string) error {


Should this live here? Also, I saw mention of a future doctor command. Do we want this migration method exported and workable with a doctor command?

Oooh, that would be nice. Let's leave it for now, and if/when the doctor command gets off the ground we can export it.

Actually, having looked at the code, I think I'd like to export it now, but not for future use in the doctor command, but rather to explicitly call the migration method from the submit command.

kytrinyx · 2018-08-28T01:24:02Z

cmd/submit_test.go

+
+	_, err = os.Stat(expectedPathAfterMigration)
+	assert.NoError(t, err)
+}


This looks good. Can we also check that there's no file at the legacy location?

kytrinyx · 2018-08-28T01:26:31Z

workspace/solution.go

+		fmt.Fprintf(os.Stderr, "\nMigrated solution metadata to %s\n", metadataPath)
+	} 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", metadataPath)


I think if they both exist, we delete the old one.

kytrinyx · 2018-08-28T01:28:21Z

workspace/workspace_test.go

 		err := os.MkdirAll(path, os.FileMode(0755))
 		assert.NoError(t, err)

-		if path != a2 {
+		if path != filepath.Join(a2, ignoreSubdir) {


I was thinking os.MkdirAll on the filepath.Dir of the full metadata file filepath -- I'd rather do that than have to have the code know about the various pieces of the metadata file path.

kytrinyx · 2018-08-28T01:33:00Z

workspace/exercise.go

@@ -37,7 +37,12 @@ func (e Exercise) Filepath() string {

 // MetadataFilepath is the absolute path to the exercise metadata.
 func (e Exercise) MetadataFilepath() string {
-	return filepath.Join(e.Filepath(), solutionFilename)
+	return filepath.Join(e.Filepath(), ignoreSubdirMetadataFilepath())


How would you feel about making the relative path to the metadata filepath a variable defined at compile time rather than a function call?

var metadataFilepath = filepath.Join(".exercism", "solution.json")

Sure. I'd made it a function call as a workaround to keep const.

Ah right. Personally, I'd rather have a var than a function call

kytrinyx · 2018-08-28T01:48:43Z

workspace/workspace.go

@@ -113,9 +113,22 @@ func (ws Workspace) SolutionDir(s string) (string, error) {
 		if _, err := os.Lstat(path); os.IsNotExist(err) {
 			return "", err
 		}
-		if _, err := os.Lstat(filepath.Join(path, solutionFilename)); err == nil {
+		if err := checkMetadataFile(path); err == nil {


I'm not quite comfortable with how this method not only checks the files, but also does the migration.

What do you think about inlining the checking logic into this function, so that it explicitly checks the metadata file path and returns if it's there, then checks the legacy metadata file path and returns if it's there.

Then, after we've found the solution directory, call a workspace.MigrateLegacyMetadataFile() function (or some such name), which is a noop if the metadata file isn't legacy.

Yeah, I wasn't happy with this either. What you've suggested seems like the way to go.

kytrinyx · 2018-08-28T01:49:41Z

workspace/solution.go

+	return nil
+}
+
+func migrateLegacySolutionFile(legacyMetadataPath string, metadataPath string) error {


Actually, having looked at the code, I think I'd like to export it now, but not for future use in the doctor command, but rather to explicitly call the migration method from the submit command.

kytrinyx

I like where this is going. It's a surprisingly small amount of code, now!

I have a few suggestions, below.

kytrinyx · 2018-08-28T01:50:47Z

cmd/submit_test.go

@@ -434,3 +483,19 @@ func writeFakeSolution(t *testing.T, dir, trackID, exerciseSlug string) {
 	err := solution.Write(dir)
 	assert.NoError(t, err)
 }
+
+func writeFakeLegacySolution(t *testing.T, dir, trackID, exerciseSlug string) {


This is only used in one place, right? If so I'd rather inline it into the test.

Right. Good call.

jdsutherland · 2018-08-28T05:14:34Z

cmd/submit_test.go

+	_, err = os.Stat(expectedPathAfterMigration)
+	assert.NoError(t, err)
+	_, err = os.Stat(exercise.LegacyMetadataFilepath())
+	assert.Error(t, err)


Added check that legacy no longer exists.

Passing

Inline creating fake legacy metadata and assert legacy location DNE after migration

Passing TODO: * if legacy & modern metadata exist, handle deletion * unit tests for migration

jdsutherland · 2018-08-28T13:07:53Z

I believe all the review concerns are now implemented. I also attempted writing unit tests for the migration (I assume we want unit tests since it's exported and potentially dangerous). I think it's ready for another round.

kytrinyx

I definitely like the change to make the migration explicit. I have a few more comments here, but I think we're close.

kytrinyx · 2018-08-28T22:34:12Z

workspace/exercise.go

+
+// MigrateLegacyMetadataFile migrates a legacy metadata to the modern location.
+// This is a noop if the metadata file isn't legacy.
+// If both legacy and modern metadata files exist, the legacy file will be deleted.


👍 😍 This is such a great comment.

kytrinyx · 2018-08-28T22:34:58Z

workspace/exercise.go

+			return "", err
+		}
+		str = fmt.Sprintf("\nMigrated metadata to %s\n", metadataFilepath)
+		fmt.Fprintf(os.Stderr, str)


I'd rather not print anything in packages other than the cmd package. Let's make this return some sort of state or conclusion (a custom type, maybe an iota?) that the command package (or whoever) can check to decide whether or not to print stuff.

kytrinyx · 2018-08-28T22:36:42Z

workspace/exercise.go

+// This is a noop if the metadata file isn't legacy.
+// If both legacy and modern metadata files exist, the legacy file will be deleted.
+func (e Exercise) MigrateLegacyMetadataFile() (string, error) {
+	var str string


The str variable doesn't seem to be doing anything at the moment. Can we ditch it?

I added it for use in the unit tests. The thought was to increase test granularity: just asserting no error isn't enough to distinguish if the legacy was moved vs deleted. Based on your feedback above, it sounds like it should be turned into some kind of status type. I'm not sure if you saw you unit tests. I'll make another comment.

kytrinyx · 2018-08-28T22:37:30Z

workspace/exercise.go

+		str = fmt.Sprintf("\nRemoved legacy metadata: %s\n", legacyMetadataFilepath)
+		fmt.Fprintf(os.Stderr, str)
+	}
+	return str, nil


I think we can get rid of the else and move everything from the else down here.

Yeah, the else can be removed using early returns. I changed that here b555a3a.

kytrinyx · 2018-08-28T22:42:37Z

workspace/exercise.go

+	metadataFilepath := e.MetadataFilepath()
+
+	if _, err := os.Lstat(legacyMetadataFilepath); err != nil {
+		return "", nil


Is the assumption here that if we get an error, it's an os.IsNotExist error? Is that a safe assumption?

Yeah, this is problematic. I replaced this by calling exercise.HasMetadata() since it contains the logic needed here. The trade-off is that this adds excess calls to MetadataFilepath() but it might be worth it for improved readability and avoiding duplication?

The trade-off is that this adds excess calls to MetadataFilepath() but it might be worth it for improved readability and avoiding duplication?

Yeah, I wouldn't worry about that. If we find that we have performance problems we can do proper profiling etc to see what's up and fix it 👍

kytrinyx · 2018-08-28T22:43:23Z

workspace/solution.go

-	visibility.ShowFile(path)
-
-	if err := ioutil.WriteFile(path, b, os.FileMode(0600)); err != nil {
+	if err = createIgnoreSubdir(dir); err != nil {


I'd probably inline this. I know createIgnoreSubdir is used in a couple of places, but it doesn't feel like an actual abstraction to me, just an accidental bit of logic that happens to have to happen twice.

Agreed. As you mentioned below, using MkdirAll makes this just 2 lines.

kytrinyx · 2018-08-28T22:44:23Z

workspace/solution.go

+func createIgnoreSubdir(path string) error {
+	path = filepath.Join(path, ignoreSubdir)
+	if _, err := os.Stat(path); os.IsNotExist(err) {
+		if err := os.Mkdir(path, os.FileMode(0755)); err != nil {


If you use MkdirAll instead of Mkdir you don't have to check that it doesn't exist first.

jdsutherland · 2018-08-29T01:22:46Z

workspace/exercise_test.go

@@ -42,3 +68,86 @@ func TestNewFromDir(t *testing.T) {
 	assert.Equal(t, "the-track", exercise.Track)
 	assert.Equal(t, "the-exercise", exercise.Slug)
 }
+
+func TestMigrateLegacyMetadataFileWithoutLegacy(t *testing.T) {


Are these tests ok? I wasn't sure if this is overboard.

Yeah, I think these are great. I'm all for explicit, thorough tests :)

You could try turning it into a single table test if you wanted to--not sure what that would end up looking like (sometimes it's better, sometimes it's worth).

WIP - will replace with status

The logic for checking metadata files in the Migration method exists in Exercise's HasMetadata(). I previously avoided using these methods because this adds duplicate calls to MetadataFilepath() but this might be worth trading for DRYing up the file checking logic.

jdsutherland · 2018-08-30T03:40:37Z

workspace/exercise.go

+	MigrationStatusRemoved
+)
+
+func (m MigrationStatus) String(e Exercise) string {


Is it ok that this takes an Exercise? I'm not sure if there's a better way to do this. Making it a method on Exercise doesn't seem right.

Is it necessary to include the metadata location?

Making it a method on Exercise doesn't seem right.

Why is that? I actually think that seems fine, but I fear that I'm missing something.

No, let's not include the metadata location.

Unify the assertions on metadata existing

'dir' makes more sense now using Exercise.NewExerciseFromDir

Replace knowledge of ignoreSubdir with encapsulated Exercise

jdsutherland · 2018-08-30T04:43:22Z

workspace/solution.go

-	visibility.ShowFile(path)
-
-	if err := ioutil.WriteFile(path, b, os.FileMode(0600)); err != nil {
+	metadataAbsoluteFilepath := NewExerciseFromDir(dir).MetadataFilepath()


I named this metadataAbsoluteFilepath rather than metadataFilepath because it would shadow the var in Solution.

jdsutherland · 2018-08-30T05:01:04Z

workspace/workspace.go

+		if _, err := os.Lstat(filepath.Join(path, metadataFilepath)); err == nil {
+			return path, nil
+		}
+		if _, err := os.Lstat(filepath.Join(path, legacySolutionFilename)); err == nil {


Is it ok that above we're using the name metadataFilepath, contrasting with legacySolutionFilename here? Will we presumably clean up all related solution/metadata incoherencies in the future rename PR?

Will we presumably clean up all related solution/metadata incoherencies in the future rename PR?

Yeah, I think it makes sense to do that separately.

Knowledge of metadata filepath doesn't require asking Exercise

Reduce needless complexity

kytrinyx

This looks great! There's just the one thing that you pointed out about the migration status String() method.

kytrinyx · 2018-09-03T17:01:46Z

cmd/submit.go

+		return err
+	}
+	if verbose, _ := flags.GetBool("verbose"); verbose {
+		fmt.Fprintf(os.Stderr, migrationStatus.String(exercise))


Oh, I see what you mean by it seems wrong that it takes an exercise! Yes, somehow I didn't see that it was the String() method.

Yes, I agree that this isn't ideal.

kytrinyx · 2018-09-03T17:02:26Z

workspace/exercise.go

+	MigrationStatusRemoved
+)
+
+func (m MigrationStatus) String(e Exercise) string {


No, let's not include the metadata location.

jdsutherland · 2018-09-04T02:07:15Z

I think it's ready. Do you want me to squash some of this?

kytrinyx · 2018-09-04T22:58:10Z

I'll go ahead and squash and merge on my end.

Thank you ever so much for your patience in working through this! ❤️ 💛 💚 💙 💜 🖤

jdsutherland · 2018-09-05T02:24:10Z

Cheers. It was a great learning experience.

nywilken · 2018-09-05T02:52:03Z

@jdsutherland great stuff on getting this in. I'm glad @kytrinyx was able to help you get this into a good place. With that said, we should probably test, maybe as a pre-release, to ensure exercises get migrated over successfully.

@kytrinyx thoughts?

kytrinyx · 2018-09-09T17:53:59Z

Yepp--we can test this by building locally:

go build -o testercism exercism/main.go

Then configuring with ./testercism configure, and then running a download and submit against the live site.

@jdsutherland want to give this a first run?

nywilken · 2018-09-10T00:55:09Z

@kytrinyx I was thinking of making a pre-release version available and asking members of the Exercism group to test as well; if they are up for it. Is that okay with you?

jdsutherland · 2018-09-10T06:01:28Z

I tested this locally and verified stderr output cases were correct without any issues. I don't have an exhaustive local workspace to test widely though.

kytrinyx · 2018-09-11T01:56:20Z

@nywilken that would be great.

nywilken · 2018-09-22T03:13:33Z

@jdsutherland @kytrinyx I pushed out an alpha release so that we can test this furhter. Please feel free to download and give it a run (https://github.com/exercism/cli/releases/tag/v3.0.10-alpha.1).

There are no breaking changes here, but I would like to document how to roll back changes once done with the testing. Seeing as 3.0.9 binaries won't find the newly migrated metadata file. I will make an official issue in the AM with testing steps.

jdsutherland · 2018-09-22T03:49:11Z

There are no breaking changes here, but I would like to document how to roll back changes once done with the testing.

A shell script could create a copy workspace to test with to avoid touching the default workspace. Are you suggesting a rollback mechanism if there is an issue after live rollout? What do you have in mind?

Side note: IIRC, we deleted the visibility package related to hidden folders in Windows. Did/should we explicitly note to verify this?

jdsutherland mentioned this pull request Aug 27, 2018

Move .solution.json to hidden subdirectory #630

Closed

jdsutherland force-pushed the metadata-hidden-subdir branch from 6845814 to 193d964 Compare August 27, 2018 10:36

jdsutherland commented Aug 27, 2018

View reviewed changes

kytrinyx reviewed Aug 28, 2018

View reviewed changes

kytrinyx requested changes Aug 28, 2018

View reviewed changes

jdsutherland force-pushed the metadata-hidden-subdir branch 2 times, most recently from 543c166 to 017a055 Compare August 28, 2018 04:36

jdsutherland commented Aug 28, 2018

View reviewed changes

jdsutherland force-pushed the metadata-hidden-subdir branch from 017a055 to 9752d62 Compare August 28, 2018 07:14

jdsutherland added 8 commits August 28, 2018 14:14

Remove hard-coded metadata filename references

93aaf80

WIP move metadata file to hidden subdir

0b763e3

Passing

WIP add legacy metadata migration

0597c94

Remove visibility package

fda69bf

Refactor tests using encapsulated metadata path

2cf12a2

Refactor TestLegacySolutionMetadataMigration

66391f4

Inline creating fake legacy metadata and assert legacy location DNE after migration

WIP refactor MigrateLegacyMetadataFile()

e0e865e

Passing TODO: * if legacy & modern metadata exist, handle deletion * unit tests for migration

Refactor relative path metadata func to var

9752d62

jdsutherland force-pushed the metadata-hidden-subdir branch from cc2f0b1 to 4c68d61 Compare August 28, 2018 12:35

WIP add unit tests for MigrateLegacyMetadataFile

05545c5

jdsutherland added 2 commits August 28, 2018 20:28

Refactor MigrateLegacyMetadataFile to separate tests

8849c19

Add test for HasLegacyMetadata

49178ce

jdsutherland force-pushed the metadata-hidden-subdir branch from 4c68d61 to 49178ce Compare August 28, 2018 13:32

kytrinyx requested changes Aug 28, 2018

View reviewed changes

jdsutherland commented Aug 29, 2018

View reviewed changes

jdsutherland added 3 commits August 29, 2018 09:33

Refactor inline ignore subdirectory location

c8de610

Refactor remove printing from migration

f244b57

WIP - will replace with status

jdsutherland added 2 commits August 30, 2018 07:33

Remove migration cache vars and useless casting

0ae92f5

Add migration status printing in verbose mode

5faa585

jdsutherland commented Aug 30, 2018

View reviewed changes

Refactor inline unnecessary vars

305edf2

jdsutherland force-pushed the metadata-hidden-subdir branch from 5d85fae to 5d4e34b Compare August 30, 2018 04:06

jdsutherland added 3 commits August 30, 2018 11:06

Make migration submit test consistent w/ unit test

5d4e34b

Unify the assertions on metadata existing

Refactor improve var name

52b7405

'dir' makes more sense now using Exercise.NewExerciseFromDir

Refactor simplify solution.Write

f074b6c

Replace knowledge of ignoreSubdir with encapsulated Exercise

jdsutherland commented Aug 30, 2018

View reviewed changes

Make migration test assertion consistent

8dbfcb5

jdsutherland commented Aug 30, 2018

View reviewed changes

Refactor inline temp

e646bc8

jdsutherland force-pushed the metadata-hidden-subdir branch from 07617a4 to 839a2d4 Compare August 31, 2018 03:11

jdsutherland added 2 commits August 31, 2018 10:12

Refactor simplify Solution.Write path knowledge

839a2d4

Knowledge of metadata filepath doesn't require asking Exercise

Refactor simplify TestWorkspaceExercises

c6bff35

Reduce needless complexity

kytrinyx reviewed Sep 3, 2018

View reviewed changes

jdsutherland added 2 commits September 4, 2018 06:44

MigrationStatus.String() doesn't take an Exercise

998b67c

Remove unnecessary Sprintf

dc2feb5

kytrinyx merged commit 5185642 into exercism:master Sep 4, 2018

jdsutherland deleted the metadata-hidden-subdir branch September 5, 2018 02:23

Metadata hidden subdir #723

Metadata hidden subdir #723

Conversation

jdsutherland commented Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsutherland Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsutherland Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kytrinyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsutherland Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsutherland commented Aug 28, 2018 • edited Loading

kytrinyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsutherland Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsutherland Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsutherland Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsutherland Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

jdsutherland Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kytrinyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdsutherland commented Sep 4, 2018

kytrinyx commented Sep 4, 2018

jdsutherland commented Sep 5, 2018

nywilken commented Sep 5, 2018

kytrinyx commented Sep 9, 2018

nywilken commented Sep 10, 2018

jdsutherland commented Sep 10, 2018 • edited Loading

kytrinyx commented Sep 11, 2018

nywilken commented Sep 22, 2018 • edited Loading

jdsutherland commented Sep 22, 2018 • edited Loading

jdsutherland commented Aug 27, 2018 •

edited

Loading

jdsutherland Aug 27, 2018 •

edited

Loading

jdsutherland Aug 28, 2018 •

edited

Loading

jdsutherland Aug 28, 2018 •

edited

Loading

jdsutherland commented Aug 28, 2018 •

edited

Loading

jdsutherland Aug 29, 2018 •

edited

Loading

jdsutherland Aug 29, 2018 •

edited

Loading

jdsutherland Aug 30, 2018 •

edited

Loading

jdsutherland Aug 30, 2018 •

edited

Loading

jdsutherland Aug 30, 2018 •

edited

Loading

jdsutherland commented Sep 10, 2018 •

edited

Loading

nywilken commented Sep 22, 2018 •

edited

Loading

jdsutherland commented Sep 22, 2018 •

edited

Loading