Skip to content
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

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

Merged

Conversation

jdsutherland
Copy link
Contributor

@jdsutherland jdsutherland commented Sep 15, 2018

Renaming workspace.Solution (and solution.json) was previously discussed here:
#630 (comment)
#607 (comment)

This change is a pure rename.

I chose workspace.ExerciseMetadata, opting for Exercise over Solution because I believe Exercise more intuitively describes the metadata.

@kytrinyx
Copy link
Member

I believe Exercise more intuitively describes the metadata.

I agree. Giving this a review now, but I'd love to get @nywilken input on it as well (even though it's a pure rename, thus not complicated, it's an important rename to get right, I think!)

Copy link
Member

@kytrinyx kytrinyx left a comment

Choose a reason for hiding this comment

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

Overall I think the choice of ExerciseMetadata is really great. I have reservations about ExerciseMetadatas, as detailed below.

Also, I think we need to rename some local variables that are currently s.

@@ -144,7 +144,7 @@ func TestDownload(t *testing.T) {

dir := filepath.Join(targetDir, "bogus-track", "bogus-exercise")
b, err := ioutil.ReadFile(workspace.NewExerciseFromDir(dir).MetadataFilepath())
var s workspace.Solution
var s workspace.ExerciseMetadata
Copy link
Member

Choose a reason for hiding this comment

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

I think s is probably the wrong variable name now that the type has been renamed. How would you feel about metadata to match the variable names you've used in the matching command file? Since it's tests, we could also do something shorter (em? meta?). I don't know. I like the consistency of metadata.

}
var s Solution
var s ExerciseMetadata
Copy link
Member

Choose a reason for hiding this comment

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

Here, too, I think we need something other than s.

@@ -47,11 +47,11 @@ func NewSolution(dir string) (*Solution, error) {
// Suffix is the serial numeric value appended to an exercise directory.
// This is appended to avoid name conflicts, and does not indicate a particular
// iteration.
func (s *Solution) Suffix() string {
func (s *ExerciseMetadata) Suffix() string {
Copy link
Member

Choose a reason for hiding this comment

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

Likewise here (and throughout this file). I think in this case em is good, since it's in the context of a method receiver.

dir, err := ioutil.TempDir("", "solution")
assert.NoError(t, err)
defer os.RemoveAll(dir)

s1 := &Solution{
s1 := &ExerciseMetadata{
Copy link
Member

Choose a reason for hiding this comment

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

Here, too.

package workspace

// ExerciseMetadatas is a collection of exercise metadata to interactively choose from.
type ExerciseMetadatas []*ExerciseMetadata
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super happy about Metadatas as a plural. Data is already plural. How about something like MetadataCollection or ExerciseMetadataCollection?

@@ -17,7 +17,7 @@ func TestNewSolutions(t *testing.T) {
filepath.Join(root, "bravo"),
filepath.Join(root, "charlie"),
}
sx, err := NewSolutions(paths)
sx, err := NewExerciseMetadatas(paths)
Copy link
Member

Choose a reason for hiding this comment

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

I think we need another variable name here, as well (but I don't have any suggestions until we settle on the type name).

@jdsutherland
Copy link
Contributor Author

jdsutherland commented Sep 17, 2018

I think the issues have been addressed.

When changing these short var names I noticed something unrelated to the rename of this PR:

-func (ws Workspace) ExerciseDir(s string) (string, error) {
-	if !strings.HasPrefix(s, ws.Dir) {
+func (ws Workspace) ExerciseDir(path string) (string, error) {
+	if !strings.HasPrefix(path, ws.Dir) {
 		return "", errors.New("not in workspace")
 	}
 
-	path := s
+	currentPath := path
 	for {
-		if path == ws.Dir {
+		if currentPath == ws.Dir {
 			return "", errMissingMetadata
 		}
-		if _, err := os.Lstat(path); os.IsNotExist(err) {
+		if _, err := os.Lstat(currentPath); os.IsNotExist(err) {
 			return "", err
 		}
-		if _, err := os.Lstat(filepath.Join(path, metadataFilepath)); err == nil {
-			return path, nil
+		if _, err := os.Lstat(filepath.Join(currentPath, metadataFilepath)); err == nil {
+			return currentPath, nil
 		}
-		if _, err := os.Lstat(filepath.Join(path, legacyMetadataFilename)); err == nil {
-			return path, nil
+		if _, err := os.Lstat(filepath.Join(currentPath, legacyMetadataFilename)); err == nil {
+			return currentPath, nil
 		}
-		path = filepath.Dir(path)
+		currentPath = filepath.Dir(currentPath)
 	}
 }

I think s is ambiguous here. Do you think this is worth changing? If so, should it be part of this PR?

@jdsutherland jdsutherland force-pushed the solution.json-rename-to-metadata branch 2 times, most recently from aff1ed7 to 2c73d54 Compare September 17, 2018 02:10
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

@jdsutherland thanks for tackling this. I left a couple of change requests for you within my review, but will have some additional comments in response to @kytrinyx review.

I also called out an example and the Go spec, which I think could be helpful as you learn Go. The spec is a little dense so please feel free to ask any questions.

@@ -10,14 +10,14 @@ import (
"time"
)

const solutionFilename = "solution.json"
const legacySolutionFilename = ".solution.json"
const metadataFilename = "metadata.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add a note for this in the release notes as it could cause confusion for any users who version control their workspace files.

b, err := ioutil.ReadFile(filepath.Join(dir, metadataFilepath))
if err != nil {
return &Solution{}, err
return &ExerciseMetadata{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing as our return value is a pointer we can return nil, err not bother allocating memory for an empty ExerciseMetadata.

return &Solution{}, err
var metadata ExerciseMetadata
if err := json.Unmarshal(b, &metadata); err != nil {
return &ExerciseMetadata{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

The same holds true here: return nil, err


// NewExerciseMetadataCollection loads up the exercise metadata for each of the provided paths.
func NewExerciseMetadataCollection(paths []string) (ExerciseMetadataCollection, error) {
var metadataCollection []*ExerciseMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing as you defined a custom type for a slice of ExerciseMetadata you should use that because while the following two variables are indeed a slice of ExerciseMetadata they are actually not the same type. Named types ExerciseMetatadataCollection are their own type, regardless of what their underlying type []*ExerciseMetadata is.

Here is an example of what I am talking about
https://play.golang.org/p/_LuoH39ODUq

This concept is a little tough to explain in a review so take a minute to read the spec, and ask me any questions about it. https://golang.org/ref/spec#Type_identity

@nywilken
Copy link
Contributor

@jdsutherland @kytrinyx I think the names make sense. ExerciseMetadataCollection seems right right to me; it is easily understood what it represents.

However, do we actually need it? I seems like we only have this type being used in a test. In fact, the constructor is so simple that I would say when we need to handle multiple paths we can achieve that functionality without a custom type. In short, I see we remove it!

@jdsutherland
Copy link
Contributor Author

jdsutherland commented Sep 18, 2018

@nywilken Thanks for the review. The feedback gives the impression that it was viewed through the lens of additions rather than a pure rename. I think the suggestions are spot on though.

Looking at the history, ExerciseMetadataCollection (previously workspace.Solutions) was added by @kytrinyx as a way to implement interactive selection but it looks like this was moved to a comms package that was eventually removed in 1bb612f. Maybe workspace.Solutions should have been removed as part of those changes?

@nywilken
Copy link
Contributor

The feedback gives the impression that it was viewed through the lens of additions rather than a pure rename.

I have the tendency to do this a bit, otherwise I’m left thinking that I may have missed something.

I think the suggestions are spot on though.

Glad to hear that. The refactor work you got goning on here is good so happy to provide guidance anyway possible.

@kytrinyx
Copy link
Member

I think s is ambiguous here. Do you think this is worth changing? If so, should it be part of this PR?

@jdsutherland I think s is fine here, though path is clearer. If you'd like to change it, I think it should go in a separate PR.

However, do we actually need it? I seems like we only have this type being used in a test.

@nywilken That's a great catch! I have been simplifying and refactoring, and didn't notice that we don't use it yet.

@jdsutherland how would you feel about deleting it in a separate PR to be merged before this one? (I can also make the change, but not today or tomorrow).

@nywilken
Copy link
Contributor

Looking at the history, ExerciseMetadataCollection (previously workspace.Solutions) was added by @kytrinyx as a way to implement interactive selection but it looks like this was moved to a comms package that was eventually removed in 1bb612f. Maybe workspace.Solutions should have been removed as part of those changes?

I thought that was the case. I grokked the code to make sure there were no other usage of this type, but didn’t say out right to remove. In case, there was something I was missing. Prior to our current implementation we had the ability to do an interactive submit which handled directories and multiple files so this made sense then. Now that we have a simple submit, let’s remove the ExerciseMetadatCollection type along with its tests.

BTW thanks for pulling up the history.

@nywilken
Copy link
Contributor

how would you feel about deleting it in a separate PR to be merged before this one?

@jdsutherland I’ll submit a PR for the deletion and work with with @kytrinyx to have it merged prior to this one.

@nywilken
Copy link
Contributor

@kytrinyx @jdsutherland sorry for the confusion. @jdsutherland if you have the bandwidth right now to delete the type please do. I thought that was your response to @kytrinyx. Otherwise I can get to it later tonight. My apologies.

@jdsutherland jdsutherland force-pushed the solution.json-rename-to-metadata branch from e1f61b0 to 92f5e0d Compare September 18, 2018 01:22
@jdsutherland
Copy link
Contributor Author

@nywilken I'll make a PR for the deletion now.

@nywilken
Copy link
Contributor

@nywilken I'll make a PR for the deletion now.

🙇‍♂️

@nywilken
Copy link
Contributor

@jdsutherland I merged #737 you should be able to rebase onto master to get the latest changes.

@jdsutherland jdsutherland force-pushed the solution.json-rename-to-metadata branch from 7572a18 to 65283cd Compare September 18, 2018 01:45
@jdsutherland
Copy link
Contributor Author

@nywilken done

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

@jdsutherland this looks good to me. @kytrinyx if all is good with you I will merge this in.

CHANGELOG.md Outdated
@@ -5,7 +5,7 @@ The exercism CLI follows [semantic versioning](http://semver.org/).
----------------

## Next Release
* **Your contribution here**
* [#736](https://github.com/exercism/cli/pull/736) Metadata file .solution.json renamed to metadata.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jdsutherland we normally add the contributor name to the changelog entry - [@jdsutherland].

@nywilken
Copy link
Contributor

I prepared the branch https://github.com/nywilken/exercism-cli/tree/aplha-release-branch-3.0.10, which I plan to make available as a pre-release for testing the renamed metadata file. I want to wait until this change is merged because I noticed that if I migrate an exercise to .exercism/solution.json it wont actually get renamed to .exercism/metadata.json because the legacy check is specific to track/slug/.solution.json

@jdsutherland
Copy link
Contributor Author

jdsutherland commented Sep 19, 2018

I want to wait until this change is merged because I noticed that if I migrate an exercise to .exercism/solution.json it wont actually get renamed to .exercism/metadata.json because the legacy check is specific to track/slug/.solution.json

Maybe I'm misunderstanding what's being said here. The state of the code of the branch isn't expected to rename to metadata.json, it renames .solution.json to solution.json https://github.com/nywilken/exercism-cli/blob/2c1e6c8d7c6b899617690bc48c25bb6b0fb641d0/workspace/solution.go#L13-L14

@nywilken
Copy link
Contributor

nywilken commented Sep 19, 2018

@jdsutherland sorry for the confusion. If I pre-release master as it is right now it will migrate (rename) .solution.json to .exercism/solution.json which is expected.

However, if I ask the community to test it and then merge in your branch where we rename solution.json to metadata.json. Users who tested with 3.0.10-alpha will not actually see the rename because it will not get flagged as a legacy file.

So as to not have to write more code to handle that case I would rather wait to get this PR merged so that we go from .solution.json to .exercism/metadata.json

@nywilken nywilken merged commit 2fbd8d7 into exercism:master Sep 22, 2018
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.

3 participants