Skip to content

isbn-verifier: Fix typo in README#1171

Closed
knepprath wants to merge 4 commits intoexercism:masterfrom
knepprath:knepprath/isbn_readme_typo
Closed

isbn-verifier: Fix typo in README#1171
knepprath wants to merge 4 commits intoexercism:masterfrom
knepprath:knepprath/isbn_readme_typo

Conversation

@knepprath
Copy link
Copy Markdown

@knepprath knepprath commented Jan 20, 2018

Used configlet to update README to the canonical problem description for isbn-verifier.

@knepprath knepprath changed the title Fix typo in README isbn-verifier: Fix typo in README Jan 20, 2018
@cmccandless
Copy link
Copy Markdown
Contributor

If we're fixing the README for this exercise anyway, perhaps we should conform to the canonical problem description?

@knepprath
Copy link
Copy Markdown
Author

Good insight! I'm new to the project and have only been working with Python. I didn't realize there was a canonical description shared across languages.

Maybe the README for every exercise should simply redirect to the canonical problem description. Seems like a fools errand trying to keep them all synchronized every time there is an update.

@cmccandless
Copy link
Copy Markdown
Contributor

The intent behind maintaining separate copies is to allow each track to include track-specific information (how to run tests, implementation details, etc.). However, there is a tool that generates README files for a track using the canonical descriptions.

@knepprath
Copy link
Copy Markdown
Author

@cmccandless, Thanks! This was a fun exercise for me to get deeper in to the project.

I ran configlet on all the exercises and found 11 more that had diverged. What is the strategy for updating? Should I go ahead and do a single PR to update all of them?

@cmccandless
Copy link
Copy Markdown
Contributor

Depends on what you mean by "diverged". I know of several that have a section on Exceptions which is unique to this track; these would remain as they are.

I think it would be fine to do a single PR.

Copy link
Copy Markdown
Contributor

@N-Parsons N-Parsons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one minor issue to be resolved and then I think this can be merged.

The first 9 digits in the ISBN have to be between 0 and 9.
The check digit can additionally be an 'X' to allow 10 to be a valid check digit as well.
* Generate valid ISBN, maybe even from a given starting ISBN.
## Exception messages
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a blank line before this heading?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

On a side note. I did a spot check of a few of the description.md files in problem-specifications, and none of them end with a newline. So seems like all README's generated with the configlet will have similar formatting issue. Would we want the configlet to programmatically handle this?

@cmccandless
Copy link
Copy Markdown
Contributor

780fcee solved this issue, so I am going to close this PR.

@knepprath, regarding the whitespace issue you observed: as all README files in this track are generated as of #1179, editing individual README files by hand no longer makes sense. It would be best to determine which template file should include the additional blank line, create a PR to add it, and also regenerate all READMEs (same PR as template change). If you are interested in pursuing this issue, I would recommend taking a look at this track's README template.

@cmccandless
Copy link
Copy Markdown
Contributor

@knepprath Actually, I believe the whitespace issue has been resolved already. If not, I am currently working on a PR to add another section to the track template, and will resolve it there.

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 this pull request may close these issues.

3 participants