lint: Check for README presence (and supporting commits) #98
Conversation
| @@ -31,6 +32,11 @@ func NewExercise(root string, pg PatternGroup) (Exercise, error) { | |||
| return ex, err | |||
| } | |||
|
|
|||
| err = setPath(root, "README\\.md", &ex.ReadmePath) | |||
There was a problem hiding this comment.
I'm not seeing a need to let this be configurable, so it is not in PatternGroup
petertseng
force-pushed
the
readme
branch
2 times, most recently
from
3e36a32 to
9838396
Compare
There was a problem hiding this comment.
@petertseng this looks good - thanks for adding this check. I left a few comments on the changes.
My (unsubstantiated) assumption is that they will be required to be
present after Nextercism, as we don't want to keep generating READMEs on
the fly.
...
(Reviewers should confirm this assumption. If it is incorrect, this PR
is inappropriate.)
I have not confirmed this requirement for Nextercism, but I think that README files should be required just as a solution or test-suite. This requirement, it addition to the point you called out, will help ensure READMEs stay inline with the exercise, especially exercises not part of the default problem spec, and provide guidance to future contributors. However, this requirement comes at a cost for new tracks, as all metadata and template files must exist in order to generate a README.
If track documentation clearly outlines the requirements than this should be a non-issue. With that said, we should check which tracks are missing readmes and open issues/PRs for those tracks before releasing.
cmd/lint_test.go
Outdated
| slugs := missingReadme(track) | ||
|
|
||
| if len(slugs) != 2 { | ||
| t.Fatalf("Expected missing solutions in 2 exercises, missing in %d", len(slugs)) |
fixtures/numbers/config.json
Outdated
| @@ -24,6 +24,12 @@ | |||
| "difficulty": 1 | |||
| }, | |||
| { | |||
| "uuid": "444", | |||
There was a problem hiding this comment.
If we can avoid adding new exercises to this fixture the better. The numbers track is meant to be used as an example track to illustrate command usage and error reporting. If you want to add a failing test case I recommend you add it under the fixtures/lint/... with a track name specific to the test case. But to be honest, I don't think it is necessary given the tests you already have in place, which I believe you also mentioned in the PR description.
| @@ -31,6 +32,11 @@ func NewExercise(root string, pg PatternGroup) (Exercise, error) { | |||
| return ex, err | |||
| } | |||
|
|
|||
| err = setPath(root, "README\\.md", &ex.ReadmePath) | |||
|
For my records only, no need to review yet: 9e6c660 = solution->readme copy/paste error fixed, but exercise 444 still present |
As of trackler 2.2.1.45 (out of date, 2.2.1.46 is out): exercism/discussions#200 (comment) For reasons, that needed to be changed. 515e2329092261591647130ccc05413acbee7458 = solution->readme copy/paste error fixed, but exercise 444 still present At a72782d, exercise four was moved to a different track. I am conflicted on whether to do this because the other lint checks do not have their own individual fixture, but I was thinking that something should check whether this lint happens, whether that be the example test (by placing it in I didn't yet check the statement "added test would fail without code change", need a few hours to do that. At this point it can be reviewed again. |
Checked, it does (after I fix the path to the dir in the test, and add the UUID) |
There was a problem hiding this comment.
@petertseng thanks for updating the PR. I have just one minor nitpick before merging.
Exercise zero under fixtures/numbers/exercises is marked as foregone within fixtures/numbers/config.json so the added README can be safely removed, as it is not used within the tests.
After exercism/meta#15, READMEs are to be generated and placed into each track's exercise implementation directory. This may even become required at some point. Prepare for this requirement by placing READMEs in the correct places. Note: numbers/zero does not require a README as it is foregone.
After exercism/meta#15, READMEs are to be generated and placed into each track's exercise implementation directory. My (unsubstantiated) assumption is that they will be required to be present after Nextercism, as we don't want to keep generating READMEs on the fly. If this assumption is correct, it seems necessary to check that READMEs are present on all exercises. With the attached fixture change and attached test changes: * The added TestMissingReadme would fail if the attached code change were incorrect. * The added TestLintTrack case on missing-readmes would fail without the attached code change. Closes exercism/discussions#200 Closes exercism/configlet#86
Very nice. OK, it is removed. FYI, I haven't yet received confirmation from exercism/discussions#200 that READMEs are indeed be required in theory, and I have confirmation from exercism/discussions#200 (comment) that they are currently not required in practice. It is still possible there are plans to have them be required in the future! I leave this decision in y'all's hands, but I like to reiterate that I'm not in a hurry to get this merged before there is confirmation. |
|
I'm surprised to find that they are not required. Are they auto-generated from the fetch command if they are not in the exercise? |
According to the documentation here an exercise should contain a README file, which describes the exercise to a user. If a README does not exist, for a particular exercise, it will be automatically generated by trackler. So I think it is safe to say that a README is required in order for an exercise to be served via Exercism but, as it currently stands, not a hard requirement for Seeing as we have been adding support for different metadata variations (i.e MixedCaseName, Title, SnakeCaseName) I vote for making |
| func missingReadme(t track.Track) []string { | ||
| readmes := map[string]bool{} | ||
| for _, exercise := range t.Exercises { | ||
| readmes[exercise.Slug] = exercise.HasReadme() |
There was a problem hiding this comment.
Might check and only add if non-exist to make it faster for the second iteration since tracks generally will have readmes. And also for foregone, we can delete added keys. In the end, we can iterate smaller map and make a slice without checking.
There was a problem hiding this comment.
Great suggestion. There's a couple of lint checks that could benefit from this refactor.
There was a problem hiding this comment.
I really was thinking that missingReadme, missingSolution, missingTestSuite should really all be refactored into one function that parameterises over HasReadme, IsValid (what a name...), HasTestSuite respectively. They're all the same except for that one line, I think. And then this improvement can be applied to them all.
I plan to cut a new release tomorrow, for the tree command and readme generation enhancements. After the release I would like to merge this PR in and work with track maintainers to ensure the required READMEs are in place before releasing. Please let me know if there are any objections. |
Yes for Trackler-based Exercism. Nextercism instead generates it at import time and stores it in the DB.
This is why I liked to specify being required in theory and required in practice. If documentation says it "should" exist but there are neither any justification for that "should" nor any negative consequences for it not to exist, I can't stand behind it. A premise I hold: Configlet exists to help track maintainers know ahead of time whether their changes will break Nextercism and Exercism. When I consider whether a lint should be in Configlet, I judge it by that premise. If Configlet lints for something that is not required in practice, it is being unnecessarily restrictive. With that in mind:
I think I have changed to take on a stronger stance. I request that this PR should only be merged if there is confirmation that Nextercism will, at some point in the future, require READMEs in practice. A strong motivating factor for me: Since I maintain three tracks, I have 3x the maintenance burden when it comes to having READMEs in my tracks. If I deleted all READMEs and simply accepted that the READMEs students get will be the auto-generated ones, I immediately remove all that maintenance burden. Currently, I can justify the maintenance burden with "I guess it makes it easier for contributors to language tracks to ensure that READMEs contain in fact what they should contain", so for now I am content to let the READMEs stay. But if the README maintenance burden ever becomes too much for me, I may feel it is necessary to delete them all. If configlet starts requiring READMEs, this avenue of relief for me will be closed. And removing this avenue is unnecessary if READMEs are actually not required in practice. So, if I need to reduce my maintenance burden and this PR is merged and READMEs are still not required in practice, it may be the case that I will need to place certain lint checks behind feature flags ( There are other avenues of relief I can seek:
So, it is likely that "I need to reduce my maintenance" burden will be false, and therefore it would be safe to merge this PR. But if you see any problem in my reasoning and therefore think it would not be safe to merge, then let's talk about it. |
|
From my perspective the READMEs should be required. It's an oversight on my part that we didn't already discuss adding it to the |
|
@kytrinyx thanks for clarifying. |
|
@petertseng functionality wise this is good. Let us know if you have any other changes you wish to make. Regarding your comment about consolidating the three checks, I think it could add a-bit of complexity for the check and test code base. But if you want to submit an alternate PR for review that would be cool to. Thanks again for submitting this enhancement. |
None planned in this PR |
|
@petertseng If you wish, I can give a try for the consolidation improvement. |
Okay, it's all yours. |
|
@kytrinyx @petertseng I would like to move forward with merging this. I've been a bit removed from the project, but would like to get back on things. Please let me know if anything has changed during my unexpected hiatus. |
|
From what I see in:
the result of a missing README is that the default README will be used. Since the role of configlet is to prevent tracks from making mistakes that would break the site, but a missing README does not break either site, I am obligated to classify a README requirement in configlet as extraneous. (An easy counter to this argument is "we obviously don't want to break the site yet, but let's add this check to prepare for the future where the site does require a README", but I was going to leave this argument as an exercise to the reader) Not only is this check currently extraneous, it also removes an option currently available to overloaded track maintainers who wish for relief (deleting all READMEs and accepting the default generated README). Thus, it is against the track maintainers' interests. As a track maintainer who acts in my self-interest, I am obligated to oppose the merging of this PR. Obviously I cannot change the fact that I have already written this code and that it is out there in the world. I could force push an empty commit onto this branch to "be difficult", but that would unnecessarily display an anti-cooperative spirit. It would also be futile because anyone with this branch checked out would easily recover the code. So the best I can do is leave it be. So, y'all can do with this code as you wish, but I have no interest in supporting this PR any further. |
|
V2 does not deliver a README in the exercise if the README is missing. The code that you linked to above is for showing the description on the site. That doesn't help people when fetching the code via the CLI. I left support for default readme in Trackler because I didn't want the site to break as we migrated exercises across. Now that exercises are migrated, I'd like a PR that adds an exercise but doesn't add a README to break the build. I would like to land this PR. |
|
Oh! In that case it is prudent for me to update my position, since my previous opinion was based on a mistaken premise. Well, we certainly are agreed that it is desirable to deliver a README to the students! It is still true that having to keep READMEs up to date in individual tracks is a significant burden, but this is a problem to be solved elsewhere. Any opposition here would be directed at the wrong target. (It is, however, still the case that if y'all wish any further changes to be made to this PR that y'all will have to make them yourselves. But it seems like no further changes are necessary at the moment) |
|
@kytrinyx @petertseng thank you for the help and detailed comments. I gave the code another look over and rebased locally to test. This PR is good to go. Below are the tracks that would need to be fixed before this feature is released. I created issue #116 to track these updates. |
After exercism/meta#15, READMEs are to be
generated and placed into each track's exercise implementation
directory.
My (unsubstantiated) assumption is that they will be required to be
present after Nextercism, as we don't want to keep generating READMEs on
the fly.
If this assumption is correct, it seems necessary to check that READMEs
are present on all exercises.
(Reviewers should confirm this assumption. If it is incorrect, this PR
is inappropriate.)
With the attached fixture change and attached test changes:
were incorrect.
the attached code change.
Closes exercism/discussions#200
Closes exercism/configlet#86