-
-
Notifications
You must be signed in to change notification settings - Fork 23
Generate list of unimplemented exercises for a track #149
base: main
Are you sure you want to change the base?
Conversation
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.
I took a quick look at this conceptually (I will take a look at the code side of things in a separate pass).
Conceptually I want to make sure that we're actually defining/testing all the cases that we need (over the years, exercise definitions have grown quite complex!).
I think the use cases that we need to cover are:
- happy path 1: the problem specification exists (present, valid, not deprecated), and the exercise is implemented in the track
- happy path 2: the problem specification does not exist, and the exercise is implemented in the track with custom metadata in
.meta
- missing specification: the problem specification does not exist, there is no custom specification in
.meta
, but the track has an implementation. The message should be that there is no specification.
This last thing might mean any number of things:
- is there a typo in the exercise directory name?
- is this a completely new exercise? If so, should it be submitted as a new, generic exercise to Exercism's problem specifications? Or should this be a custom exercise just for this track? I'm not sure what the error message should be in this case; maybe just "no specification found", and we can figure out documentation later.
Then we have the deprecation stories (assume the exercise exists both in the problem specification repo and the track repo).
- the problem specification is deprecated, and the exercise is deprecated. No message. (The curriculum team has decided that the exercise is not a good one, and the track maintainers have done the work to deprecate it. It's all good.)
- the problem specification is not deprecated, the exercise is deprecated. No message. (The problem specification is fine, but the track maintainers have decided it's not a good fit for the track. It's all good).
- the problem specification is deprecated, the exercise is not deprecated. Needs a message. In this case we think this exercise is not a good fit for the platform, and the maintainers should consider deprecating it.
Then there is the foregone
story, which you've covered: we have an implementation, but the track maintainers said this is not a good fit for the track, so they labeled it as foregone
.
I think that's all the use cases we have at the moment. Eventually there may be something about versioning, where the canonical data has been updated but the implementation in the track doesn't reflect the new changes, but I don't think we've actually formalized any metadata around how the tracks keep track of this.
Update: It looks like there are primitives for keeping track of versions based on the canonical data. Can you confirm how these are specified within the track implementation? What happens if the implementation in the track doesn't specify a version at all?
{ | ||
"exercise": "not-implemented", | ||
"version": "1.0.0" | ||
} |
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.
Is this how not-implemented exercises are represented? I'm thinking, let's say that the Python track decided to implement an exercise apple-pie
. If the problem specifications repository has an apple-pie
directory, I would expect the exercise to be implemented. It might not have canonical data (but that's maybe fine; I think the canonical data is optional).
Another use case would be that there is an apple-pi
exercise (some sort of play on the mathematical concept of "pi"), but then a track tries to implement apple-pie
(type). It would not find a directory with that name in the problem specifications. Would it be correctly reported as not implemented?
{ | ||
"exercise": "up-to-date", | ||
"version": "2.0.0" | ||
} |
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.
Do exercises in the problem specifications repo have a config.json
file?
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.
Ok! I took a look at the code. I think we need to make sure we're actually sure what we're trying to do here and all the various cases we're checking before going into the implementation too much!
"github.com/spf13/cobra" | ||
) | ||
|
||
var pathExampleProblemSpecifications = "<path/to/problemspecifications>" |
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.
Nitpick: I think it would be worth calling the directory problem-specifications
here, since that is what the repository is named. If people clone it directly, then that is what their directory name will be called.
|
||
For example, if upstream prepares a new exercise it is important to either | ||
implement it in a track or declare it foregone. If a new version is published the update | ||
should be made available soon. |
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.
Rather than say "for example", let's clarify the ways in which the implementation would typically need attention.
Also, I think it's important to consider whether the exercise is a good fit for the track, but not necessarily to implement it.
So maybe something like:
When a new exercise is added to the problem specifications repository it is available to be implemented in the language tracks.
When a problem specification is deprecated, track maintainers should consider deprecating the corresponding implementation.
When the canonical data of a problem specification is updated, the corresponding implementation may need to be updated.
I wonder what we will do in the case where the maintainer decides that they don't want to make the suggested change. I think we should think this through before we finalize this functionality, as this would probably break the builds without giving people a way out.
|
||
Args: cobra.ExactArgs(2), | ||
|
||
Example: fmt.Sprintf(" %s implementation-status %s %s", binaryName, pathExampleProblemSpecifications, pathExample), |
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.
Let's give the problem specification path a default value. My guess is that most people would stick it in a directory parallel to the track directory:
.
├── problem-specifications
└── the-track
That way if they are using the default they don't need to actually say anything here.
Another thing to consider is how this command would be run on Travis. Will we need to clone the problem-specifications repository to run it? (Probably). Do we already do this? (Maybe). What would the command look like in that case? (No idea!)
) | ||
|
||
// listProblemSpecs gives a map of exercise name and whether the exercise is deprecated. | ||
func listProblemSpecs(path string) (map[string]int, error) { |
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.
If problem specifications have both deprecation status and version information, we may want to make this an array of a custom struct type.
That would help with the clarity of the function name as well. The name list
is a bit ambiguous (a list is not a map). I think a map is right, but maybe a map of slug to problem specification (an actual struct value with a deprecation field).
ret[f.Name()] = implementationStatusOK | ||
} else { | ||
ret[f.Name()] = implementationStatusDeprecated | ||
} |
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.
We need to handle the case where it's an error, but not an os.IsNotExist
error.
} | ||
|
||
func trackConfigExercises(track string) (map[string]int, error) { | ||
var trackConfig struct { |
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.
We read the track config elsewhere in this program, right? Should this be a package type?
return ret, nil | ||
} | ||
|
||
func trackConfigExercises(track string) (map[string]int, error) { |
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.
Here, too, the name is pretty ambiguous. I wonder if a map of name to status is a bit too cryptic here. Maybe we need a type for this, too. Or maybe the name should just be something like statusByExerciseSlug
to clarify the idea that it's a map of statuses.
const ( | ||
implementationStatusOK = iota | ||
implementationStatusDeprecated | ||
implementationStatusForegone |
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.
I noticed in the comment for the command that it talks about versions. Is this being implemented?
Is this still being worked on? If not, I'd be thrilled to pick up where it was left off and polish it up for merging. |
You've got my vote. 😄 |
Mine too! Sorry for letting this slide, I'm currently unable to find time for this... thanks @Smarticles101 for picking up. |
@thriqon no problem! I'll open a new pull request and build off your existing code. :) |
fixes #144.