curriculum icon indicating copy to clipboard operation
curriculum copied to clipboard

feat(mdlint): add rule TOP009 to ensure lesson overview items start with a capital letter and end with a period

Open nik-rev opened this issue 1 year ago • 15 comments

Adds a new rule to markdown lint.

Lesson overview items should end with a period and start with a capital letter

Issue

Closes https://github.com/TheOdinProject/curriculum/issues/27625#issuecomment-2112275087

Additional Information

Pull Request Requirements

  • [x] I have thoroughly read and understand The Odin Project curriculum contributing guide
  • [x] The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • [x] The Because section summarizes the reason for this PR
  • [x] The This PR section has a bullet point list describing the changes in this PR
  • [x] If this PR addresses an open issue, it is linked in the Issue section
  • [x] If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • [x] If any lesson files are included in this PR, they follow the Layout Style Guide

nik-rev avatar May 15 '24 14:05 nik-rev

The checks aren't running due to a merge conflict. You'll need to rename your rule and relevant files/dirs to use TOP009 (including the markdownlint config) because #27915 added what's now TOP008. That PR also adjusted the markdownlint config, so that should be resolved once you resolve the merge conflicts.

In terms of testing your rule, you'll need to make sure your test files include all of the different scenarios to test, and you can run the lint script on it locally. Once you're happy with that, then a new push to here (after resolving conflicts) will trigger the actions with your local linting files.

Thanks for letting me know! I've fixed the merge conflicts. Now I just need to fix the code so that it works

nik-rev avatar May 15 '24 15:05 nik-rev

A heads up that the merge has wiped out your original doc file, so you'll want to restore it but under the TOP009 name of course

mao-sz avatar May 15 '24 15:05 mao-sz

A heads up that the merge has wiped out your original doc file, so you'll want to restore it but under the TOP009 name of course

Yay! I got it working

nik-rev avatar May 15 '24 15:05 nik-rev

A heads up that the merge has wiped out your original doc file, so you'll want to restore it but under the TOP009 name of course

Thankfully we have git for that :D

nik-rev avatar May 15 '24 15:05 nik-rev

Question - my understanding from the original issue was that this rule was meant to prevent LO items being questions, as they instead should be statements of what the lesson will cover. But this rule tries to enforce the opposite - is this intended? I see that the doc file implies the rule is supposed to prevent questions (LO items must begin capitalised and end with a .)

mao-sz avatar May 15 '24 15:05 mao-sz

Question - my understanding from the original issue was that this rule was meant to prevent LO items being questions, as they instead should be statements of what the lesson will cover. But this rule tries to enforce the opposite - is this intended?

Oh, this was just a mistake on my part. I just fixed it!

nik-rev avatar May 15 '24 16:05 nik-rev

Actually, seems like there are still some mistakes. Sorry let me test it a bit more.

nik-rev avatar May 15 '24 16:05 nik-rev

Question - my understanding from the original issue was that this rule was meant to prevent LO items being questions, as they instead should be statements of what the lesson will cover. But this rule tries to enforce the opposite - is this intended? I see that the doc file implies the rule is supposed to prevent questions (LO items must begin capitalised and end with a .)

The tests are passing as expected now. I just accidentally searched for the 2nd character instead of the 1st character. .at(1). I have been writing code in another language where the array indexes start with 1 so I didn't think twice about this

nik-rev avatar May 15 '24 16:05 nik-rev

For the changes that would be better made on my computer than via the github online editor, I'll add them in a few weeks time since I have exams at the moment.

nik-rev avatar May 24 '24 22:05 nik-rev

For the changes that would be better made on my computer than via the github online editor, I'll add them in a few weeks time since I have exams at the moment.

Not a problem at all - there's no rush for this rule for sure, and even if there somehow was, your exams are more important so please do focus on those first and foremost! Whatever changes are needed for this PR, you can certainly sort them out when you're done and have the time for this, since this is all voluntary. We have a TOP010 rule in review as well, but we can always merge that first if appropriate then just resolve the merge conflict in the custom rules array when you get back to this later.

mao-sz avatar May 24 '24 22:05 mao-sz

Not sure what's going on here, I've removed the comment but that didn't seem to work

image

used to work just fine before

nik-rev avatar Jun 29 '24 18:06 nik-rev

@MaoShizhong Thank you! The PR is not ready for review yet as the tests are not passing. I will let you know when they are.

nik-rev avatar Jun 30 '24 16:06 nik-rev

Had a think about this further.

I think we should add a non-script-fixable error if an LO item ends with a ? that reports something like "Lesson overview items must be statements, not questions."

If I wrote - What are linters? then the current code would only complain about the ? and simply fixing (script or manual) to - What are linters. will not make sense, yet the linter will be happy.

I think questions should be flagged as such and not made script-fixable, as that requires changing the sentence wording itself e.g. to - Describe what linters are.

Then in that situation, we can flag and script-fix capitalisation/end punctuation errors. Those errors must only flag if the question error doesn't.

I agree with you. I think introducing fixers only makes sense when they are beneficial in every case. I don't think it makes sense for there to be a punctuation auto fixer in the first place. For example, while Describe what linters are -> Describe what linters are. makes sense, What are linters -> What are linters. would not make as much sense because the sentence itself is phrased as a question, only proper capitalisation fixer makes sense to me describe what linters are -> Describe what linters are, what do you think

nik-rev avatar Jul 09 '24 13:07 nik-rev

I think I'm with you on that, where the nuances and complexity of a fixer catching all cases isn't really that beneficial given the example you provided.

I would probably think then we just need the one general error reporting something like "Lesson overview items must be statements, not questions, and must begin with a capital letter and end with a period." No need for actual/expected, just provide the LO item itself in the context property.

That should flag for questions (ends in ?), as well as uncapitalised/un-period-ended LO items. Then we leave it to the user to manually fix, since it's context-dependent.

mao-sz avatar Jul 09 '24 13:07 mao-sz

I think I'm with you on that, where the nuances and complexity of a fixer catching all cases isn't really that beneficial given the example you provided.

I would probably think then we just need the one general error reporting something like "Lesson overview items must be statements, not questions, and must begin with a capital letter and end with a period." No need for actual/expected, just provide the LO item itself in the context property.

That should flag for questions (ends in ?), as well as uncapitalised/un-period-ended LO items. Then we leave it to the user to manually fix, since it's context-dependent.

I made the lint rules behave like your suggestions:

  • Single warning message Lesson overview items must be statements, not questions, and must begin with a capital letter and end with a period.

nik-rev avatar Jul 14 '24 18:07 nik-rev