Skip to content
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

Add custom replace rule to markdownlint configuration #17988

Merged
merged 6 commits into from
Sep 5, 2022

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Jul 5, 2022

There are many unwanted characters that we've been replacing time to time. For example, #16324, #16770 (comment), #17834 (review) are some of the PRs.
Because of some locales or keyboard settings contributors introduce such characters.

We want to handle cases such as:

The PR adds a custom markdownlint rule/plugin markdownlint-rule-search-replace to address such issues.

With the search-replace rule in place we can run the linter like this:

# to flag the issues
yarn lint:md

# or to fix automatically
yarn fix:md

We can provide plain string or regex pattern to search and replace. So many complex cases can be handled.
Refer the rule documentation for more information.

To see the plugin in action refer #18210.


cc/ @teoli2003 , @nschonni , @caugner

@OnkarRuikar OnkarRuikar requested review from schalkneethling and a team as code owners July 5, 2022 15:07
@teoli2003
Copy link
Contributor

We have to pay attention to two things:

  • exceptions. For example, there is a page that speaks about curly braces, and this shouldn't be replaces
  • for...of, for...in, etc. These 3 dots shouldn't be replaced by an ellipsis.

So I think we should be conservative or have a way to skip pages (for the first case). And we should skip code snippets (areas enclosed with 1 or 3 backticks)

Finally, like always we must clean MDN first, before adding automation. As this was already cleaned not too long ago, this shouldn't be too difficult.

@nschonni

This comment was marked as outdated.

package.json Outdated Show resolved Hide resolved
@OnkarRuikar
Copy link
Contributor Author

#12316 probably needs to be addressed first,

Currently developers refer to only .markdownlint.json from mdn/content for the markdown linting. And they install markdownlint-cli globally themselves.
As the linting happens manually at the moment and it is not affecting any other workflows, we can merge this PR to make the feature available. We do not have to block this PR for #12316.

@OnkarRuikar OnkarRuikar force-pushed the lint-search-replace branch from fcdf906 to 8f3c613 Compare July 11, 2022 04:03
This was referenced Jul 11, 2022
@OnkarRuikar OnkarRuikar changed the title Add replace rule to markdownlint task Add custom replace rule to markdownlint configuration Jul 11, 2022
@schalkneethling schalkneethling removed their request for review July 11, 2022 10:56
@OnkarRuikar OnkarRuikar force-pushed the lint-search-replace branch from 8f3c613 to 5945053 Compare July 20, 2022 15:56
@OnkarRuikar OnkarRuikar force-pushed the lint-search-replace branch from 5945053 to 13ead1c Compare July 29, 2022 08:30
@Josh-Cena Josh-Cena added the infra Infrastructure issues (npm, GitHub Actions, linting) for this repo label Aug 16, 2022
package.json Outdated Show resolved Hide resolved
@OnkarRuikar OnkarRuikar mentioned this pull request Aug 25, 2022
@OnkarRuikar
Copy link
Contributor Author

All, I've rebased and updated the lock file. Also added new rules as per our recent experiences.

Added new rules to fix

  • internal links that are not relative
  • trailing spaces (markdownlinter doesn't flag/fix more than one trailing space)

Using this we've merged following PR

@teoli2003
Copy link
Contributor

teoli2003 commented Aug 26, 2022

Can you rebase this PR? There are merge conflicts and warnings that I think were fixed elsewhere.

@teoli2003
Copy link
Contributor

teoli2003 commented Aug 26, 2022

This PR will flag these legitimate uses of curly braces forever, or am I wrong?
Capture d’écran 2022-08-26 à 09 56 33

@teoli2003
Copy link
Contributor

Or maybe we should remove this rule from the guide as it will automatically be fixed by Markdownlint. @dipikabh @wbamberg, any thoughts?

@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Aug 26, 2022

This PR will flag these legitimate uses of curly braces forever, or am I wrong?

Yes.

There are two options to solve this:

@teoli2003
Copy link
Contributor

I think we should use HTML escape sequences in these rare cases: see #17988

@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Aug 31, 2022

All the lint errors, that were being flagged by this plugin, have been fixed.

yarn.lock Outdated Show resolved Hide resolved
@nschonni
Copy link
Contributor

I'm not sure if https://github.com/DavidAnson/markdownlint/blob/main/README.md#optionscustomrules is available in the config without migrating to the the CLI2 https://github.com/DavidAnson/markdownlint-cli2#markdownlint-cli2jsonc. Reason I haven't switched to the CLI2 is the lack of a --fix flag on the commandline, so you need a whole other config object (that could inherit from a base), which seems awkward.

I really like the quote rules and the URL ones, I'm a little -0 on the elipsis and m-dash ones.

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner September 1, 2022 06:03
@OnkarRuikar OnkarRuikar requested review from dipikabh and removed request for a team September 1, 2022 06:03
@github-actions github-actions bot added the Content:CSS Cascading Style Sheets docs label Sep 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2022

Preview URLs

Flaws

None! 🎉

External URLs

URL: /en-US/docs/Web/CSS/border
Title: border
on GitHub

No new external URLs

(this comment was updated 2022-09-01 06:07:07.801741)

@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Sep 1, 2022

I'm not sure if DavidAnson/markdownlint@main/README.md#optionscustomrules is available in the config...

Actually, we are using https://github.com/igorshubovych/markdownlint-cli project for our linting and not https://github.com/DavidAnson/markdownlint (not directly).

And markdownlint-cli does have option (-r) to specify custom rule package:

markdownlint -r markdownlint-rule-search-replace --fix "mymdfile.md"

The PR does work. I've deliberately committed the typos to the PR in deleberate error for testing commit.
And it has flagged the errors as expected:
search_replace_demo


I'm a little -0 on the elipsis and m-dash ones.

ellipsis - we are not adding any rule related to ellipsis in this PR.
m-dash - the rule will only work on text content not code blocks. All m-dashes have been fixed in the repo at the moment, so it's good to go.

@github-actions github-actions bot removed the Content:CSS Cascading Style Sheets docs label Sep 1, 2022
@dipikabh
Copy link
Contributor

dipikabh commented Sep 2, 2022

Or maybe we should remove this rule from the guide as it will automatically be fixed by Markdownlint. @dipikabh @wbamberg, any thoughts?

Yes, I think we can remove that rule from the guide (just like we've done for the things Prettier automatically fixes).

Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

LGTM, but let's not merge this during the weekend as we are not here to deal with unexpected problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Infrastructure issues (npm, GitHub Actions, linting) for this repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants