Conversation
ExerciseMetadataCollection makes sense to me. |
| } | ||
|
|
||
| solution := workspace.Solution{ | ||
| solution := workspace.ExerciseMetadata{ |
There was a problem hiding this comment.
Would it be too much of a change, confusion, if we renamed this to metadata? If we can't change the name with out changing a ton of code right now what do you think about adding a TODO?
There was a problem hiding this comment.
I went back and forth on Metadata vs ExerciseMetadata and was worried that Metadata was too unspecific. I like the shorter name better, though, so I'll go with that.
There was a problem hiding this comment.
Sorry for not being explicit when I mentioned renaming this. I meant just changing solution to metadata. I'm on the fence about the name. ExerciseMetadata is clearer, but longer.
| } | ||
|
|
||
| sx, err := workspace.NewSolutions(dirs) | ||
| sx, err := workspace.NewMetadataCollection(dirs) |
There was a problem hiding this comment.
Lets call this one collection like you do in the tests.
There was a problem hiding this comment.
@kytrinyx I left a couple of comments around renames, but otherwise this is good with me. It makes things clearer for sure. I will try and work with @jdsutherland to get his PR in to avoid having to make too many changes in the future.
What we previously called Solution is actually the exercise metadata, which also includes the user's solution UUID. In order to keep things as clear as possible, this also renames the Solutions type to MetadataCollection. I couldn't think of anything better than collection to deal with the multiples of a multiple.
kytrinyx
force-pushed
the
rename-solution
branch
from
8989657 to
ef4ce77
Compare
|
@nywilken I changed the name to |
|
@jdsutherland I think that's probably the easiest way forward. I actually think that with the recent changes, and what you already figured out in #630 it should be easier this second time around. (I'm sorry for the extra work, though!) |
|
@kytrinyx No problem at all - it's useful to learn from how you improved things. |
| } | ||
|
|
||
| solution := workspace.Solution{ | ||
| metadata := workspace.Metadata{ |
There was a problem hiding this comment.
Something about this type is off to me , but I can't put my finger on why. It may have to do with the two properties IsRequester and `Team.
nywilken
left a comment
nywilken
left a comment
There was a problem hiding this comment.
@kytrinyx not to make this review longer, but what do you think about holding off on the rename for now. I get the rationale for the change, but I'm just not sure if Metadata minimizes the confusion. I think ExerciseMetadata as the name reads better, but it is still a little confusing since the metadata file is .solution.json
I know that we will eventually refactor so if renaming to Metadata makes sense for now. Then we can roll with it. FYI I didn't leave any comments on the non rename changes made to open because #696 gets rid of it.
|
@nywilken I don't feel strongly either way. I'll close this and we can re-evaluate after the file gets moved. |
|
Awesome. 🙌🏽 |
3 participants
This is a pure rename, so while the diff is quite big, the change is small:
workspace.Solutionis nowworkspace.ExerciseMetadataworkspace.Solutionsis nowworkspace.MetadataCollectionWhat we previously called Solution is actually the exercise metadata, which also
includes the user's solution UUID.
In order to keep things as clear as possible, this also renames the Solutions
type to MetadataCollection. ExerciseMetadataCollection seemed to long,
and I couldn't think of anything better than collection to deal with the
multiples of a multiple.