-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
feat(mdlint): add rule TOP009 to ensure lesson overview items start with a capital letter and end with a period #27977
feat(mdlint): add rule TOP009 to ensure lesson overview items start with a capital letter and end with a period #27977
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 |
Thankfully we have git for that :D |
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 |
Oh, this was just a mistake on my part. I just fixed it! |
Actually, seems like there are still some mistakes. Sorry let me test it a bit more. |
Co-authored-by: MaoShizhong <[email protected]>
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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 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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two little nits but otherwise I'm happy with this 🔥 Seems to work great!
...t/TOP009_lessonOverviewItemsSentenceStructure/TOP009_lessonOverviewItemsSentenceStructure.js
Outdated
Show resolved
Hide resolved
Co-authored-by: MaoShizhong <[email protected]>
For consistency Co-authored-by: MaoShizhong <[email protected]>
Co-authored-by: MaoShizhong <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this now - let's see if @thatblindgeye has anything further to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
Adds a new rule to markdown lint.
Lesson overview items should end with a period and start with a capital letter
Issue
Closes #27625 (comment)
Additional Information
Pull Request Requirements
location of change: brief description of change
format, e.g.Intro to HTML and CSS lesson: Fix link text
Because
section summarizes the reason for this PRThis PR
section has a bullet point list describing the changes in this PRIssue
section