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

Override markdownlint-cli2 config for zh-cn #14246

Merged
merged 7 commits into from
Aug 12, 2023

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Jul 15, 2023

Related to OnkarRuikar/markdownlint-rule-search-replace#109

Better approach is to override unwanted rules by providing separate .markdownlint-cli2.jsonc config file in the language sub folder.

The new config files in language directories extend the root config file and remove/add new rules. This gives us more freedom in linting each language.

zh-cn : The new file files/zh-cn/.markdownlint-cli2.jsonc doesn't include curly quote rules.

zh-tw: The new file files/zh-tw/.markdownlint-cli2.jsonc adds new rules to replace double quotes with 「『』」 as per the first request in the comment. @yin1999 let me know if I've got the rules correct.

@OnkarRuikar OnkarRuikar requested review from a team as code owners July 15, 2023 06:05
@OnkarRuikar OnkarRuikar requested review from yin1999 and removed request for a team July 15, 2023 06:05
@queengooborg
Copy link
Collaborator

I was honestly hoping to avoid this setup because the individual rules have to be redefined in numerous files...

@OnkarRuikar
Copy link
Contributor Author

I was honestly hoping to avoid this setup because the individual rules have to be redefined in numerous files...

On the flip side, as each language has different styles, you may have to replicate rules just to bring language specific variations, like zh-tw needs angled quotes instead of curly double quotes.

To keep it clean, it's far better to have separate files and let corresponding maintainers do what they want. I strongly believe once we make such separation the code owners will add more rules as per their language.
Also we don't add new rules that frequently so replication is a small inconvenience.

@yin1999
Copy link
Member

yin1999 commented Jul 15, 2023

Hi @OnkarRuikar. Is it possible to make rule entries in search-replace inheritable and overridable. That means we can inherit the search-replace entries at the root and override anyone of them by adding an entry of the same name.

files/zh-tw/.markdownlint-cli2.jsonc Show resolved Hide resolved
files/zh-tw/.markdownlint-cli2.jsonc Outdated Show resolved Hide resolved
files/zh-tw/.markdownlint-cli2.jsonc Outdated Show resolved Hide resolved
files/zh-cn/.markdownlint-cli2.jsonc Outdated Show resolved Hide resolved
files/zh-cn/.markdownlint-cli2.jsonc Outdated Show resolved Hide resolved
@OnkarRuikar OnkarRuikar requested a review from yin1999 July 16, 2023 06:23
@OnkarRuikar
Copy link
Contributor Author

Hi @OnkarRuikar. Is it possible to make rule entries in search-replace inheritable and overridable. That means we can inherit the search-replace entries at the root and override anyone of them by adding an entry of the same name.

Unfortunately it is not possible. The main markdownlint library treats entire "search-replace": {} config as one rule; it doesn't merge the inside content; it simply overrides the entire object. So for each language we have to provide all the required rules in the corresponding .jsonc config file.

@github-actions github-actions bot added the merge conflicts 🚧 This pull request has merge conflicts that must be resolved. label Aug 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot added system Infrastructure and configuration for the project merge conflicts 🚧 This pull request has merge conflicts that must be resolved. and removed merge conflicts 🚧 This pull request has merge conflicts that must be resolved. labels Aug 5, 2023
Copy link
Member

@yin1999 yin1999 left a comment

Choose a reason for hiding this comment

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

The localized rules look good to me. Thanks @OnkarRuikar

files/zh-cn/.markdownlint-cli2.jsonc Outdated Show resolved Hide resolved
files/zh-tw/.markdownlint-cli2.jsonc Outdated Show resolved Hide resolved
Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

I still don't like this approach of overriding the entire search-replace rule, but it's better to have something, so I'll approve and merge this.

@queengooborg queengooborg merged commit 49d6818 into mdn:main Aug 12, 2023
8 of 9 checks passed
@queengooborg
Copy link
Collaborator

Well...we've already run into an issue: we also have to provide the overrides for docs/zh-cn and docs/zh-tw, and it doesn't look like "extends": "../../files/zh-cn/.markdownlint-cli2.jsonc" is going to cut it.

I'm going to work on implementing the logic proposed in OnkarRuikar/markdownlint-rule-search-replace#109.

@OnkarRuikar
Copy link
Contributor Author

Well...we've already run into an issue: we also have to provide the overrides for docs/zh-cn and docs/zh-tw, and it doesn't look like "extends": "../../files/zh-cn/.markdownlint-cli2.jsonc" is going to cut it.

Could you please elaborate on the issue? Do we need different definitions for a rule in a same locale as well?

@queengooborg
Copy link
Collaborator

No, we don't need different definitions for the docs folder -- what I mean is, the extends property isn't working.

@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Aug 14, 2023

we also have to provide the overrides for docs/zh-cn and docs/zh-tw, and it doesn't look like "extends": "../../files/zh-cn/.markdownlint-cli2.jsonc" is going to cut it.

Ahh I see. Technically, as the docs directory is under different hierarchy we we simply need to copy files/zh-cn/.markdownlint-cli2.jsonc to the docs/zh-cn folder. Same for zh-tw.

Or we separate lint commands for each locale in package.json:

"lint:md" : "lint-zh-cn:md; lint-es:md; lint-ru:md ...",
"lint-zh-cn:md": "markdownlint-cli2 --config .markdonwlint-zh-cn.config docs/zh-cn/*.md files/zh-cn/**/*.md"
"lint-es:md": "markdownlint-cli2 --config .markdonwlint-es.config docs/es/*.md files/es/**/*.md"
...

This way we'll be keeping only one copy of a configuration file for each locale.

I'm going to work on implementing the logic proposed in OnkarRuikar/markdownlint-rule-search-replace#109.

Putting intricacies of all the locales in one config fie will be complex to handle, i.e. for every locale specific rule you add you'll have to put other locales(docs and files both folders separately) in the ignore lists. It's easy to maintain different config files as it won't affect other locales if any side effects happen.

There are already two ways provided by markdownlint-cli2 tool to use different configuration for specific files. One, provide config file as an option to the command. Second, use extends feature in the config file. Manipulating the tools and adding third way is not a good idea.
I still believe this is a one time setup and we are not going to change or add rules that frequently after this. So we should tolerate a little duplication.

@LeoMcA
Copy link
Member

LeoMcA commented Aug 14, 2023

If it's a choice between duplication and complexity, I'd err on the side of duplication in this case.

Or we separate lint commands for each locale in package.json:

If it's possible to only separate out the locales which need it, I think that would be preferable - though maybe we'd want a command-per locale anyway to allow linting per locale - would it still be possible to have a shared file for most locales, and specific ones where they're required?

To make keeping the configs in sync easier, I'd suggest adding some comments to the base config to break up the replace rules into a list of "common" rules and a list of "non-common" rules, with the same reflected in the locale-specific files. Something like:

base config:

rules: [
  // rules common across all locales, if you change these change them in X and Y too:
  {
  ...
  },
  // rules specific to all locales except X and Y:
  {
  ...
  }
]

locale specific config:

rules: [
  // rules common across all locales, don't change these here, change them in the base config:
  {
  ...
  },
  // rules specific to this locale:
  {
  ...
  }
]

@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Aug 14, 2023

@LeoMcA

though maybe we'd want a command-per locale anyway to allow linting per locale - would it still be possible to have a shared file for most locales, and specific ones where they're required?

There might be a bug in markdownlint-cli2. The extends property doesn't work when the config file is provided using --config CLI option. I have a fix ready and I've started a discussion there.

At the moment only two solutions are possible:
a) Building on top of the work done so far, simply copy files/zh-cn/.markdownlint-cli2.jsonc under docs/zh-cn/.markdownlint-cli2.jsonc. Do the same for each locale that requires a separate config. In this approach all the locale specific config files inherit the common root level .markdownlint-cli2.jconc.

b) Maintain entire config files for each locale that require special treatment under lint-configs directory:

package.json
lint-configs
  /.markdownlint-cli2.json     -- config file for locale that don't need special treatment
  /zh-cn.markdownlint-cli2.json    -- entire separate config files for each locale that require different rules.
  ...

And use separate commands for each locale:

"lint:md" : "lint-zh-cn:md; lint-es:md; lint-es:md ...",
"lint-zh-cn:md": "markdownlint-cli2 --config lint-configs/.markdonwlint.config docs/zh-cn/*.md files/zh-cn/**/*.md"
"lint-es:md": "markdownlint-cli2 --config .markdonwlint.config docs/es/*.md files/es/**/*.md"

This involves no inheritance, each config file maintains complete set of rules.

c) The third approach has been implemented here: #15106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants