Skip to content

[Roman Numerals] Approach docs fixes - #4289

Open
Yrahcaz7 wants to merge 4 commits into
exercism:mainfrom
Yrahcaz7:roman-numerals-approach-fixes
Open

Yrahcaz7 wants to merge 4 commits into
exercism:mainfrom
Yrahcaz7:roman-numerals-approach-fixes

Conversation

@Yrahcaz7

Copy link
Copy Markdown
Contributor

As discussed in issue 4197. The more substantial edits (such as rewording paragraphs of pose) will be in a separate PR.

Covered audit items
  • Overall:

    • Single vs double quotes
    • Unnecessary assertions
  • introduction.md:

    • Line 20: "concept" should probably be "process" or similar.
    • Lines 24 and 95 should probably be separated from the last list item.
    • Line 91 is missing punctuation at the end.
  • if-else/content.md:

    • Lines 17, 20, 22, 24, 26, and 35: There is an extra space after the +=.
    • Lines 28 and 37: There is an extra space after the second >.
    • Line 64 is missing a serial comma in "hundreds, tens and units".
    • Line 69 does not make much sense where it is currently, so it should be moved to after the codeblock.
  • table-lookup/content.md:

    • Line 17 (75 in introduction.md): "we need" should be "get" and "our" should be "the".
    • Line 34: This header should be "Variation 1" or similar.
  • loop-over-romans/snippet.txt:

    • This snippet uses single letter variable names (that are not roman numerals).
    • Line 2 (line 30 in loop-over-romans/content.md) has an unnecessary assertion.
  • loop-over-romans/content.md:

    • Line 19: "Arabic (decimal)" should be "Arabic (decimal) numbers".
    • Lines 33-36 and 55-57 use single letter variable names (that are not roman numerals).
    • Line 40: "the" is unnecessary here.
    • Lines 44 and 45 have inconsistent spacing.
    • Lines 48 and 74 have docstrings.
    • Line 66: "1, 5, 10" should be "1, 5, and 10".
    • Line 69 is missing a serial comma in "10's, 5's and 1's".
    • Line 97: "is doing" should probably be "does".
    • Line 113 does not apply to the last variant. This should be clarified.
    • Line 113: There is a missing space after "while".
    • Line 117 is a bit awkward, changing "so need" to "so they need" should improve it.
    • Line 122: There should be an "is" after the "and".
    • Line 124: "IDE's" should be "IDEs". Also, the subject of whether VSCode is an IDE should probably be avoided.
  • recurse-match/content.md:

    • Line 22: The first part of this sentence could be improved to: "A major limitation is the lack of tail-call optimization".
    • Line 26 has an extra space before the ending period.
    • Line 41 has an extra space after convert.
    • Line 57: This link is invalid as it uses loop-over-roman instead of loop-over-romans.
  • recurse-match/content.md:

    • Lines 44-46: It is not clear what the ltr variable represents from its name. It should probably be renamed to roman_val or similar.
  • itertools-starmap/content.md:

    • Line 50 is a bit awkward; perhaps it could be changed to: "Roman numerals for 5,000 and 10,000 are not defined, so spaces are used here instead."
    • Line 63: There should be a comma after "For example".
    • Line 71: "compute" should probably be "compute()".
  • config.json:

    • Line 23: "Numerals" should be lowercased here.

Remaining audit items
  • Overall:

    • "Loop over Romans" does not make sense, it should be changed to "Loop over Roman Numerals".
    • Some approaches use type hints, which are usually not included.
  • introduction.md:

    • Lines 20-24: This paragraph should be made more generic; right now it seems to focus too much on the "With if conditions" approach.
    • Line 92 is a bit awkward.
    • Line 122: This is an odd place for the link, perhaps it should be moved to line 133.
    • Lines 135-147 should probably be reworded to use a link to the relevant lines instead of a codeblock.
    • Lines 175-180: This codeblock is formatted in a way that is very hard to read.
    • The introduction is missing a section that links to the itertools.starmap() approach.
  • if-else/content.md:

    • The first codeblock has unnecessary > 0 and > 4 comparisons.
    • Line 49: This paragraph does not actually explain how the approach works.
    • Line 61 needs more explanation.
  • table-lookup/content.md, table-lookup/snippet.txt, and introduction.md:

    • This approach uses single letter variable names (that are not roman numerals).
  • table-lookup/content.md:

    • Perhaps the comments could be merged into the rest of the explanation.
  • loop-over-romans/content.md:

    • The exact workings of most of the variants are not explained.
    • There are no headers separating the variants.
    • Lines 121-124 should probably either be:
      • removed entirely, or
      • moved to a single variant (and preserve the type hints on only that variant) and also have links to resources about type hinting.
  • recurse-match/content.md, recurse-match/snippet.txt, and introduction.md:

    • This approach uses type hints.
    • The comma in [ROMAN_NUM[idx],] is unnecessary, as it is a single-element list, not a tuple.
    • The last case (case [num, idx, digits]:) can be changed to a default case (case _:).
  • itertools-starmap/content.md and itertools-starmap/snippet.txt:

    • This approach uses type hints and single letter variable names.
  • config.json:

    • Line 53: Maybe "the recently-introduced" should be removed, as Python pattern matching has been around for almost 5 years now.

@BethanyG

Copy link
Copy Markdown
Member

OOH! Thank you for this! ⭐ 🎉

I'll review as soon as I can. 💙

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.

2 participants