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

nikitarevenco · 2024-05-15T14:45:38Z

Adds a new rule to markdown lint.

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

Because

This PR

Issue

Closes #27625 (comment)

Additional Information

Pull Request Requirements

I have thoroughly read and understand The Odin Project curriculum contributing guide
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
The Because section summarizes the reason for this PR
The This PR section has a bullet point list describing the changes in this PR
If this PR addresses an open issue, it is linked in the Issue section
If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
If any lesson files are included in this PR, they follow the Layout Style Guide

MaoShizhong

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.

nikitarevenco · 2024-05-15T15:16:27Z

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

MaoShizhong · 2024-05-15T15:20:32Z

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

nikitarevenco · 2024-05-15T15:54:56Z

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

nikitarevenco · 2024-05-15T15:55:37Z

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

MaoShizhong · 2024-05-15T15:57:53Z

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 .)

…uestion mark

nikitarevenco · 2024-05-15T16:00:11Z

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!

nikitarevenco · 2024-05-15T16:01:13Z

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

nikitarevenco · 2024-05-15T16:06:50Z

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

MaoShizhong

Great work @nikitarevenco 🚀

A few nits, a few things to consider and a few little bugs to address, but otherwise, the bulk of this is 🔥

@thatblindgeye - would also appreciate your run through of this whenever you get the chance, in case you had any further insights or ideas.

...t/TOP009_lessonOverviewItemsSentenceStructure/TOP009_lessonOverviewItemsSentenceStructure.js

MaoShizhong · 2024-05-16T17:01:09Z

...t/TOP009_lessonOverviewItemsSentenceStructure/TOP009_lessonOverviewItemsSentenceStructure.js

+          context,
+          fixInfo: {
+            lineNumber,
+            editColumn: 2,


You're going to want to check editColumn for the fixer

MaoShizhong · 2024-05-16T17:01:29Z

...t/TOP009_lessonOverviewItemsSentenceStructure/TOP009_lessonOverviewItemsSentenceStructure.js

+          context,
+          fixInfo: {
+            lineNumber,
+            editColumn: context.length,


You'll also want to check the editColumn here as well

markdownlint/docs/TOP009.md

markdownlint/TOP009_lessonOverviewItemsSentenceStructure/tests/TOP009_no_punctuation.md

markdownlint/TOP009_lessonOverviewItemsSentenceStructure/tests/TOP009_capital_letter.md

MaoShizhong · 2024-05-16T17:26:50Z

...t/TOP009_lessonOverviewItemsSentenceStructure/TOP009_lessonOverviewItemsSentenceStructure.js

+          fixInfo: {
+            lineNumber,
+            editColumn: context.length,
+            deleteCount: 1,


While a delete count of 1 might suffice for most realistic situations, what about in the instance of

- Lesson overview item?. - Lesson overview item???. - Lesson overview item...

Don't necessarily have to overcomplicate for every possible edge case but it probably wouldn't be that much more complex to handle just chopping off all chars past the last letter, instead of hard coding a deleteCount of 1.

First edge case example also questions whether just checking for the last char being . is sufficient, since ending with ?????????!?!?!?!. would count as valid. @thatblindgeye Thoughts on whether it'd be significantly valuable to flag anything that ends with more punctuation than just a single .? e.g. ?. .. .?. !.!.etc. and chop off all trailing punctuation before replacing with a single.`?

I think that would be good. Not sure if I'd block the PR over that right now (assuming that something like that would at least be caught in manual review), but it'd be a nice tweak to the rule.

Maybe just check whether the last character is a period and that the character before the period is a letter/number?

Testing for \w\.$ is simple enough, though in terms of the fixer, it'd have to count the trailing punctuation to provide the correct fixInfo.deleteCount number. Just tested and providing a higher number than there are chars in the rest of the line unfortunately throws an error. No quick and dirty deleteCount: Infinity sadly!

Edit: ignore the ping from what this message was. Realised I was looking at some projects instead of lessons, so technically their "lesson overview" sections can be whatever the hell they want since they're not enforced by the lesson linting config. Can't find any actual lessons with links in LO items, so that part should be fine.

thatblindgeye

Awesome job on this 🔥

...t/TOP009_lessonOverviewItemsSentenceStructure/TOP009_lessonOverviewItemsSentenceStructure.js

thatblindgeye · 2024-05-18T12:05:05Z

...t/TOP009_lessonOverviewItemsSentenceStructure/TOP009_lessonOverviewItemsSentenceStructure.js

+      if (!lastCharacterIsValid) {
+        onError({
+          lineNumber,
+          detail: "Lesson overview items should end with a period.",


Suggested change

detail: "Lesson overview items should end with a period.",

detail: "Lesson overview items must end with a period.",

Though also maybe we could check whether the item ends with a question mark, and make the error message, "Lesson overview items should end with a period and be written as a statement rather than a question." That isn't a blocker, though, so I'm fine with just the single message.

I think that's a good idea!

thatblindgeye · 2024-05-18T12:11:42Z

markdownlint/docs/TOP009.md

+
+## Rationale
+
+Make sure lesson overview items follow a consistent structure. Lesson overview items should be brief overviews of what the lesson is about, they should not be questions.


Suggested change

Make sure lesson overview items follow a consistent structure. Lesson overview items should be brief overviews of what the lesson is about, they should not be questions.

Lesson overview items should follow a consistent structure. They must tell the user what the lesson is about via brief overview statements, and must not be conveyed in the form of a question that the user is expected to be able to answer after completing the lesson.

Or something like that? WDYT @nikitarevenco @MaoShizhong

Think we could chop off the "that the user is expected to be able to answer after completing the lesson" bit, so just the following reads 👌 to me.

Lesson overview items should follow a consistent structure. They must tell the user what the lesson is about via brief overview statements, and must not be conveyed in the form of a question.

Yeah, that's better 100%

thatblindgeye · 2024-05-18T12:13:06Z

markdownlint/TOP009_lessonOverviewItemsSentenceStructure/tests/TOP009_invalid_punctuation.md

+- Lesson overview item 1.
+- Lesson overview item 2.
+- Lesson overview item 3?


I feel like we could combine this and the "no_punctuation" file into one test, since they're essentially testing for the same thing (valid punctuation).

thatblindgeye · 2024-05-18T12:21:28Z

...t/TOP009_lessonOverviewItemsSentenceStructure/TOP009_lessonOverviewItemsSentenceStructure.js

+          fixInfo: {
+            lineNumber,
+            editColumn: context.length,
+            deleteCount: 1,


I think that would be good. Not sure if I'd block the PR over that right now (assuming that something like that would at least be caught in manual review), but it'd be a nice tweak to the rule.

Maybe just check whether the last character is a period and that the character before the period is a letter/number?

thatblindgeye · 2024-05-18T12:23:29Z

...t/TOP009_lessonOverviewItemsSentenceStructure/TOP009_lessonOverviewItemsSentenceStructure.js

@@ -0,0 +1,82 @@
+// A bullet list is every token between and inclusive bullet_list_open to bullet_list_closed


I don't think we really need these comments in the file

Oh yeah, forgot to remove them - They're misleading

Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>

Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>

nikitarevenco · 2024-05-24T22:33:00Z

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.

Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>

MaoShizhong · 2024-05-24T22:42:18Z

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.

nikitarevenco added 10 commits May 10, 2024 21:00

feat(top008): add test for capital letter

b4d10b7

feat(top008): add test for invalid punctuation

0f1b271

feat(top008): add test for no punctuation

8d0558b

feat(top008): add test that is valid

956d928

feat(top008): add incomplete code

27df378

feat(top008): add to .markdownlint-cli2.jsonc

1374317

feat(top008): add doc

16720c1

feat(008): complete isolateBulletList function

929d512

feat(TOP008): init

a2cead4

feat(008): give proper name to the rule

6be62b2

MaoShizhong requested changes May 15, 2024

View reviewed changes

nikitarevenco added 3 commits May 15, 2024 16:06

feat(008): rename to 009

79fe731

Merge branch 'main' into feature/TOP008

fbe5249

fix: rename TOP008 to TOP009 in names

007c4ce

feat(009): update the docs to point towards TOP009

09859f7

nikitarevenco added 2 commits May 15, 2024 16:53

fix(009): incorrect boolean leading to linting not working

04f3a9a

feat: remove console log

b191d00

nikitarevenco marked this pull request as ready for review May 15, 2024 15:56

feat: add doc (again)

a3a1740

fix(009): lesson overview items should end with a period instead of q…

8abd278

…uestion mark

nikitarevenco marked this pull request as draft May 15, 2024 16:01

fix: finds the 1st character instead of the 2nd character

16c916c

nikitarevenco marked this pull request as ready for review May 15, 2024 16:05

nikitarevenco requested a review from MaoShizhong May 15, 2024 16:11

nikitarevenco changed the title ~~Markdown rule - lesson overview items should start with a capital letter and end with a question mark~~ feat(mdlint): new rule TOP009 => lesson overview items should start with a capital letter and end with a period May 15, 2024

nikitarevenco changed the title ~~feat(mdlint): new rule TOP009 => lesson overview items should start with a capital letter and end with a period~~ feat(mdlint): add rule TOP009 to ensure lesson overview items start with a capital letter and end with a period May 15, 2024

refactor(top009): remove console logs

e2c12ef

MaoShizhong requested changes May 16, 2024

View reviewed changes

MaoShizhong requested a review from thatblindgeye May 16, 2024 17:28

thatblindgeye requested changes May 18, 2024

View reviewed changes

nikitarevenco and others added 4 commits May 24, 2024 23:25

refactor: consistency in if statement

842e232

Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>

refactor: use kebab-caso for consistency

068a75d

Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>

fix(top009): remove language tag

d508f2c

Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>

refactor(top009): rename rule

29dfba0

Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>

nikitarevenco and others added 2 commits May 24, 2024 23:33

fix(top009): spelling to conform to american standarts

758c06f

Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>

refactor(top009): improve phrasing

68bb91a

Co-authored-by: MaoShizhong <122839503+MaoShizhong@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

nikitarevenco commented May 15, 2024 •

edited

MaoShizhong left a comment

nikitarevenco commented May 15, 2024

MaoShizhong commented May 15, 2024

nikitarevenco commented May 15, 2024

nikitarevenco commented May 15, 2024

MaoShizhong commented May 15, 2024 •

edited

nikitarevenco commented May 15, 2024

nikitarevenco commented May 15, 2024

nikitarevenco commented May 15, 2024

MaoShizhong left a comment

MaoShizhong May 16, 2024

MaoShizhong May 16, 2024

MaoShizhong May 16, 2024 •

edited

thatblindgeye May 18, 2024

MaoShizhong May 18, 2024

MaoShizhong May 18, 2024 •

edited

thatblindgeye left a comment

thatblindgeye May 18, 2024

nikitarevenco May 24, 2024

thatblindgeye May 18, 2024

MaoShizhong May 18, 2024

nikitarevenco May 24, 2024

thatblindgeye May 18, 2024

thatblindgeye May 18, 2024

thatblindgeye May 18, 2024

nikitarevenco May 24, 2024

nikitarevenco commented May 24, 2024

MaoShizhong commented May 24, 2024

	detail: "Lesson overview items should end with a period.",
	detail: "Lesson overview items must end with a period.",


		## Rationale

		Make sure lesson overview items follow a consistent structure. Lesson overview items should be brief overviews of what the lesson is about, they should not be questions.

	Make sure lesson overview items follow a consistent structure. Lesson overview items should be brief overviews of what the lesson is about, they should not be questions.
	Lesson overview items should follow a consistent structure. They must tell the user what the lesson is about via brief overview statements, and must not be conveyed in the form of a question that the user is expected to be able to answer after completing the lesson.

		@@ -0,0 +1,82 @@
		// A bullet list is every token between and inclusive bullet_list_open to bullet_list_closed

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

Are you sure you want to change the base?

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

Conversation

nikitarevenco commented May 15, 2024 • edited

Because

This PR

Issue

Additional Information

Pull Request Requirements

MaoShizhong left a comment

Choose a reason for hiding this comment

nikitarevenco commented May 15, 2024

MaoShizhong commented May 15, 2024

nikitarevenco commented May 15, 2024

nikitarevenco commented May 15, 2024

MaoShizhong commented May 15, 2024 • edited

nikitarevenco commented May 15, 2024

nikitarevenco commented May 15, 2024

nikitarevenco commented May 15, 2024

MaoShizhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaoShizhong May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaoShizhong May 18, 2024 • edited

Choose a reason for hiding this comment

thatblindgeye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitarevenco commented May 24, 2024

MaoShizhong commented May 24, 2024

nikitarevenco commented May 15, 2024 •

edited

MaoShizhong commented May 15, 2024 •

edited

MaoShizhong May 16, 2024 •

edited

MaoShizhong May 18, 2024 •

edited