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

Who/what shall be responsible for ensuring that README.md exists for each exercise? Configlet or individual tracks? #200

Closed
petertseng opened this issue Oct 12, 2017 · 10 comments · Fixed by exercism/v2-configlet#98

Comments

@petertseng
Copy link
Member

petertseng commented Oct 12, 2017

Since exercism/meta#15, README.md is present in each track's individual exercise implementation.

First, the discussion will assume the premise that it is desirable that this README.md in fact be present.

Trackler is OK with the README.md being missing: See https://github.com/exercism/trackler/blob/master/lib/trackler/implementation.rb#L51. The README is assembled in the old fashion.

I was under the impression that Nextercism would not be OK with the README.md being missing. https://github.com/exercism/prototype/blob/master/app/models/exercise.rb#L37 uses a description from the database and I might assume that the database field is only populated via the README.md from the repo, and not the README assembled in the old fashion. I have no basis for this assumption so the chance of it being wrong is very high.

If it will be required to have the README.md, where shall it be checked?

  1. configlet checks it.
  2. Each track is responsible for adding a check to its own CI.
  3. Each reviewer of a PR adding new exercise implementations must check that it is present.
  4. Your suggestion here.
  5. They aren't required. This has all been a big misunderstanding on my part.
@Smarticles101
Copy link
Member

I vote option 1.

I think it's a good idea for configlet to check for it because every track already uses configlet in the CI to check that the config.json is in order. Option 2 would cause more work to be done than necessary, and option 3 would cause even more work to be done and a missing readme could easily be overlooked by a maintainer.

@ferhatelmas
Copy link

Reasoning by @Smarticles101 seems very logical to me and I couldn't come up with a better suggestion as option 4 so I also vote for option 1.

@ilya-khadykin
Copy link

ilya-khadykin commented Oct 13, 2017

I guess this validation should be part of configlet lint command, so I'm in favor of Option 1

@petertseng
Copy link
Member Author

And, what exercises are missing READMEs?

gems/trackler-2.2.1.45$ for d in tracks/*/exercises/*/; do if [ ! -f $d/README.md ]; then echo $d; fi; done
tracks/bash/exercises/error-handling/
tracks/cfml/exercises/acronym/
tracks/cfml/exercises/atbash-cipher/
tracks/cfml/exercises/bob/
tracks/cfml/exercises/diamond/
tracks/cfml/exercises/difference-of-squares/
tracks/cfml/exercises/flatten-array/
tracks/cfml/exercises/gigasecond/
tracks/cfml/exercises/grains/
tracks/cfml/exercises/hamming/
tracks/cfml/exercises/hello-world/
tracks/cfml/exercises/isogram/
tracks/cfml/exercises/largest-series-product/
tracks/cfml/exercises/luhn/
tracks/cfml/exercises/markdown/
tracks/cfml/exercises/nth-prime/
tracks/cfml/exercises/pangram/
tracks/cfml/exercises/pig-latin/
tracks/cfml/exercises/raindrops/
tracks/cfml/exercises/rna-transcription/
tracks/cfml/exercises/saddle-points/
tracks/cfml/exercises/scrabble-score/
tracks/cfml/exercises/secret-handshake/
tracks/cfml/exercises/space-age/
tracks/cfml/exercises/sum-of-multiples/
tracks/cfml/exercises/triangle/
tracks/cfml/exercises/word-count/
tracks/csharp/exercises/complex-numbers/
tracks/dart/exercises/raindrops/
tracks/elm/exercises/collatz-conjecture/
tracks/gnu-apl/exercises/leap/
tracks/haxe/exercises/leap/
tracks/php/exercises/collatz-conjecture/
tracks/php/exercises/flatten-array/
tracks/plsql/exercises/hello-world/
tracks/python/exercises/change/
tracks/python/exercises/two-bucket/
tracks/ruby/exercises/change/
tracks/ruby/exercises/rotational-cipher/
tracks/swift/exercises/rotational-cipher/

@bdw429s
Copy link

bdw429s commented Oct 14, 2017

Yeah, I'll be honest-- I didn't include readmes in most of the CFML exercises because I didn't have anything to add or subtract from the default readme, which displays by default on the website. I figured less was more :) Having the readme delivered to the student tho is a worthy cause, so I'll go through and update all the CFML excercises to have the default readme.

@bdw429s
Copy link

bdw429s commented Oct 14, 2017

@petertseng The CFML track now has default readmes on every exercise. Please update your list.

@petertseng
Copy link
Member Author

Having the readme delivered to the student tho is a worthy cause

Luckily, the Trackler-backed Exercism would still have delivered the default READMEs, because https://github.com/exercism/trackler/blob/master/lib/trackler/implementation.rb#L40. So READMEs would only have been not-delivered for those using Nextercism.

Please update your list.

The list is automatically generated from me running the script on Trackler gem as it exists on my filesystem as delivered to me by RubyGems. I have no power to change the output of the script until Trackler changes.

Just kidding, I can grep.

gems/trackler-2.2.1.45$ for d in tracks/*/exercises/*/; do if [ ! -f $d/README.md ]; then echo $d; fi; done | grep -v cfml
tracks/bash/exercises/error-handling/
tracks/csharp/exercises/complex-numbers/
tracks/dart/exercises/raindrops/
tracks/elm/exercises/collatz-conjecture/
tracks/gnu-apl/exercises/leap/
tracks/haxe/exercises/leap/
tracks/php/exercises/collatz-conjecture/
tracks/php/exercises/flatten-array/
tracks/plsql/exercises/hello-world/
tracks/python/exercises/change/
tracks/python/exercises/two-bucket/
tracks/ruby/exercises/change/
tracks/ruby/exercises/rotational-cipher/
tracks/swift/exercises/rotational-cipher/

I'm just saying that's all I can do until Trackler is updated.

petertseng pushed a commit to petertseng/configlet that referenced this issue Oct 14, 2017
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 example test would fail without the attached code change.
* The unit test in `lint_test.go` testing against `fixtures/numbers`
  actually does not fail, because there are other reasons for the track
  to be invalid, so the attached code change was not necessary. This
  points to the unit test being too coarse, but this is a discussion for
  another day. For now, the example test suffices.

Closes exercism/discussions#200
Closes exercism/configlet#86
petertseng pushed a commit to petertseng/configlet that referenced this issue Oct 14, 2017
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 example test would fail without the attached code change.
* The unit test in `lint_test.go` testing against `fixtures/numbers`
  actually does not fail, because there are other reasons for the track
  to be invalid, so the attached code change was not necessary. This
  points to the unit test being too coarse, but this is a discussion for
  another day. For now, the example test suffices.

Closes exercism/discussions#200
Closes exercism/configlet#86
petertseng pushed a commit to petertseng/configlet that referenced this issue Oct 14, 2017
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 example test would fail without the attached code change.
* The added TestMissingReadme would fail if the attached code change
  were incorrect.
* The TestLintTrack against `fixtures/numbers` actually does not fail,
  because there are other reasons for the track to be invalid, so the
  attached code change was not necessary. This points to the unit test
  being too coarse, but this is a discussion for another day. For now,
  the other two tests suffice.

Closes exercism/discussions#200
Closes exercism/configlet#86
@petertseng
Copy link
Member Author

Looking at the current results of missing README.md:

Arbitrarily select the first exercise among active tracks (important because non-active tracks don't show on Nextercism!) from the list of those exercises missing READMEs.

Trackler-backed Exercism:

Nextercism:

Oh. Well even without being able to download nextercism download, my hypothesis of "Nextercism requires READMEs" has already been invalidated. Maybe it will be re-validated in a later version of Nextercism, I have no idea.

Depends on whether https://github.com/exercism/prototype/blob/4d093ec7766b292f014205d1747015b90b6468c5/app/services/git/syncs_track.rb#L94 is meant to be temporary or permanent.

petertseng pushed a commit to petertseng/configlet that referenced this issue Oct 16, 2017
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 example test would fail without the attached code change.
* The added TestMissingReadme would fail if the attached code change
  were incorrect.
* The TestLintTrack against `fixtures/numbers` actually does not fail,
  because there are other reasons for the track to be invalid, so the
  attached code change was not necessary. This points to the unit test
  being too coarse, but this is a discussion for another day. For now,
  the other two tests suffice.

Closes exercism/discussions#200
Closes exercism/configlet#86
petertseng added a commit to petertseng/configlet that referenced this issue Oct 16, 2017
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
petertseng added a commit to petertseng/configlet that referenced this issue Oct 16, 2017
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
petertseng added a commit to petertseng/configlet that referenced this issue Oct 18, 2017
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
@kytrinyx
Copy link
Member

At the moment nextercism assumes that the README is present in the exercise, but doesn't require it.

Configlet has a command to generate READMEs, but at the moment it needs to be run manually by the contributor or maintainer. I intend to spec out a bot that will add/update READMEs when they're missing or things change, but I don't have a clear roadmap at the moment so can't promise an ETA.

I agree with the conclusion that configlet lint is the right place for this check (and see that you've all very capably kicked that off. Thank you.)

@Stargator
Copy link

@kytrinyx I agree configlet lint seems like the best place.

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 a pull request may close this issue.

7 participants