Allow a track to define custom exercises #24
Conversation
|
This follows on from the ideas in #23. As I talked about there, the goal seems to be to make Problem open to accepting roots other than |
lib/trackler/problem.rb
Outdated
| @@ -6,15 +6,16 @@ class Problem | |||
| attr_reader :slug, :root | |||
There was a problem hiding this comment.
The attr_reader for :root will not work properly any more.
The easy fix is to just define the @root instance variable.
Given it seems to be untested it might not be used externally and should be removed, but it would be useful to double check this.
There was a problem hiding this comment.
Good catch. I don't want to change the existing API, even if it isn't used (or just isn't tested). I fixed this and amended my commit.
Insti
left a comment
Insti
left a comment
There was a problem hiding this comment.
I like these changes. 😄 Nice work, and thanks for the excellent commit message 🌟
No functional or API changes here. Just a refactoring of the internals of Problem. With just `@root` there was a some extra effort necessary to switch the various file/paths into either URLs or filesystem locations. This can be simplified by clearly splitting the concepts. Within a problem there is now a file_root and a repo_root. The former is used when looking at a filesystem, the latter for creating URLs. It's possible that these two responsibilities should live elsewhere. But that's for later. The various files contained within a problem have had their `_path` methods replaced with a `_file_name` method that can be used to build either filesystem locations or URLs. The `file_path` and `repo_url` return what you'd expect.
IanWhitney
force-pushed
the
refactor_problem_path_concepts
branch
from
dd9b4e5 to
9a0ab05
Compare
|
Do you want to continue with other PRs that build on this one without having to wait for this one to be merged? |
|
I will continue working on this until I get to a point where something interesting happens. Then we can merge. |
The snowflake track implements a custom problem, snowflake-only. The test of Tracks had to change to adjust to the new fixture. This test might be a bit brittle, but that's something to address later. Adding e new test of Track to make sure that its `problems` and `implementations` contain a the `snowflake-only` problem. These tests pass with no code changes, so there are no functional changes in this commit. However, the problem returned by these methods is wrong. If you ask the location of its files it will point you to `common`, and that is wrong. However, the Track test probably isn't the place to test that. At least not right now. I think that will change.
IanWhitney
changed the title
|
Adjusting the name and description of this PR to outline my plan and give me some milestones. |
Yeah, don't tell me that this code is ugly. I'm aware. This commit establishes tests for how a problem should behave if it has track-specific files. Note that this has _interesting_ side effects. If a track were to define their own custom metadata.yml file for a problem that exists in common, then that custom metadata.yml file would be used over the commonly defined one. This may not be what we want. We'll have to figure that out. Once we're comfortable with how to handle these situations this code will all be refactored. Which is why I'm not worried about the ugliness.
|
@kytrinyx, I'm going to pull you into this again. Os of 5a24ee3 When I create a The question is which takes precedence. As I say in my commit
Is that cool? Or should we default to Common first and Track-specific as a backup? |
|
(and yes, the code is 5a24ee3 is terrible. I know. Shameless green, right?) |
Probably a bigger refactor than was strictly necessary. But this (mostly) extracts the metadata concept from Problem and puts it into a Metadata object The `Metadata.for` method builds an appropriate metadata object. The options are: - Metadata using a file in the track - Metadata using a file in common - A NullMetadata object if no metadata.yml file exists for the problem The `metadata.exists?` thing is lifted from Track, which already uses that predicate. Problem previously defaulted to `nil` for the track param, but that doesn't work once I wanted to pass the track on to Metadata, so I've also introduced NullTrack. A nice thing about having a guaranteed track object is that I can call `track.exists?` before building the track-specific ivars.
Similar to Metadata, Descriptions can be one of three types: - A track-specific one - A common one - Non-existent Unlike `metadata`, there was already a `description` method in `problem`, so I can't have `problem.description` return the description object w/o breaking existing APIs So, the `description` method continues to return the content of the description and the private `description_object` method returns the object.
I think that is exactly as it should be. Most specific first, falling back to defaults where missing.
Absolutely! |
I wanted to remove the triple-if from Description#for, this led me to removing the file behaviors from Description entirely. Description now always returns a Description (before it returned either a Description or a NullDescription) The part of Description that varies, its content and where it comes from, is now down in the DescriptionFile logic. I've put all 4 DescriptionFile variants together in a file. I think if someone wants to look at one they'll probably want to look at the others. The classes work together, not independently. Now that Description#for no longer returns a NullDescription, the implementation of the `exists?` method needed to change. It now checks for the presence of content. If there is no description file, the content will be empty-string
I thought this would be identical to Description/Description file, but there are a few differences. MetadataFile has `to_h`, while Description has `to_s`. Because the metadata file is structured data, while the description is just text. Other than that they seem to be pretty much the same and there's probably a concept that can be extracted out of this.
The last bit of description/metadata knowlegde still living in problem was the Github URL code. This commit moves that knowledge into Metadata and Description objects. As a bonus, I no longer have to do the weird `track_repo_root` ivar and the associated conditionals in Problem. The remaining clients of `repo_root` method only work with files that exist in x-common, so I no longer have to check with github url root to use.
|
Haaaay, checking an item off the to-do list. Feels good. |
This test made sure nothing broke, but didn't exercise the behavior I want. A track's problems can include track-only ones, and track-only problems exist. ALso, checking implementations is not necessary for this test. The logic in Implementations isn't changing.
Passes the test to show a track can return track-specific problems. Now that we can get track-specific problems by providing a track to `Problem.new`, we want to make sure to create track-specific problems when building our Implementations collection for a track. Doing this requires passing the track into Implementations so that it can be passed on to Problem. This changed the arity of Implementations.new, so that test needed an update. But there are no clients of `Implementations.new` outside of this gem, so this is not a breaking change.
This test covers the following to-do item > Track#implementations contains implementations of common and track-spcecific problems There were no functional changes to get this to pass. It already did, as implementations are already track-specific
As mentioned in exercism#22 ``` Trackler.problems Current behavior: Returns a Problems collection that contains all problems in the common directory Proposed behavior: Unchanged Would it contain track-specific problems? No. ``` and ``` Trackler.implementations Current behavior: Returns a Hash. The key is a problem slug, the values is an array of all tracks that implement that problem. Proposed behavior: Same structure, but if a track implements a track-only problem, it would be contained in this Hash. So, in the example we would now have {..."rust-only" => [<rust implementation object>]...} Trackler.implementations['rust-only'] would return a an array with a single implementation object.a ``` and ``` Trackler.todos Current behavior: Returns a Hash. The key is a track, the value is an array of common problems that the track does not implement. Proposed behavior: Unchanged Would it contain track-specific problems? No. Trackler.todos['rust-only'] would return nil. ``` These tests exercise these behaviors
|
Ok, @kytrinyx. This now completes what I laid out in #22. There is refactoring to be done, but the functionality is done and it is covered by tests. If this looks good, I'd like to
|
The behavior of these is tested through Problem, but, hey, why not test these files directly. I only wanted to test the behavior of `for` and the state of the objects it creates, so I've made a bunch of methods private, leaving only the necessary API public. This does not change the behavior of these classes, only narrows how they can be used down to the ways they _should_ be used.
I never liked DescriptionFile and MetadataFile. They were clearly doing 90% the same stuff. This commit refactors their behavior, putting the similar stuff into GuaranteedFile and moving the custom stuff back into Metadata and Description And I get to remove all the MetadataFile and DescriptionFile stuff. Fun.
|
@IanWhitney I just wanted to say that I've seen all the work you've been doing on all of this and appreciate it so much. I've had a really hectic week and my brain is pretty fried. I'm not sure if I'll get to the point where I can review code this weekend. ❤️ |
|
Oh, I'm in no rush. I know i've been pushing a bunch of commits on this and I figured I should say that the functionality was done |
kytrinyx
left a comment
kytrinyx
left a comment
There was a problem hiding this comment.
This looks really good. I think this is the right approach. I have a couple of small comments, but nothing major.
lib/trackler/implementations.rb
Outdated
| attr_reader :track_id, :repo, :slugs, :root | ||
| def initialize(track_id, repo, slugs, root) | ||
| attr_reader :track_id, :repo, :slugs, :root, :track | ||
| def initialize(track_id, repo, slugs, root, track) |
There was a problem hiding this comment.
Do we still need a track_id if we're passing in a track?
test/trackler_test.rb
Outdated
| assert_nil Trackler.problems.detect { |p| p.slug == "snowflake-only" } | ||
| end | ||
|
|
||
| def test_problems_does_not_contain_track_specific_problems |
There was a problem hiding this comment.
I think this test name is a copy/paste. It doesn't reflect the implementation of the test.
| @repo_root = "https://github.com/exercism/x-common/blob/master/exercises/%s/" % self.slug | ||
|
|
||
| @metadata = Metadata.for(problem: self, track: track) | ||
| self.description_object = Description.for(problem: self, track: track) |
There was a problem hiding this comment.
I see why we have to name it something like this, and I'm wishing (out loud) that I could think of a better name. But so far I haven't come up with anything.
There was a problem hiding this comment.
I know, right? It bugs me to no end.
exercism#24 (comment) `Implementation` still needs the track id, but we can use the provided track instance.
|
Comments addressed, I think. |
|
🎉 Thank you! |
|
Where shall we document this so that others may enjoy! |
|
It's on my to-do list. Hoping to start it tomorrow. |
|
It may also be necessary to modify https://github.com/exercism/request-new-language-track/pull/13/files#r108047475 to suit. |
5 participants
This will be the tracking issue for #22.
The To Do List:
Trackler.implementationsreturns hash that contains common and track-only exercisesWhen complete, the hash returned by this will contain track-specific
Implementationobjects inside the array. For example, if the Rust track has a track-specific exercise calledrust-only, this hash will containTrack#implementationscontains implementations of common and track-spcecific problemsTrack#problemscontains common and track-spcecific problemsProblemcan be created for a track-specific problemmetadataanddescriptionmethods return content stored within the track, notcommonmetadata_urlanddescription_urlmethods return urls for the track repo, not commoncanonical_data_urlreturns nil, as track-specific problems will not have canonical data files