-
-
Notifications
You must be signed in to change notification settings - Fork 718
-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
Integrate OnkarRuikar/markdownlint-rule-search-replace #673
Comments
That's really neat, I love it!! It's not a "normal" rule like the existing MD### rules, so my current thinking is to leave it separate. (I see it's already tagged so it will be easier to find.) /cc @OnkarRuikar |
No problem, maybe they can explain the issue with disabling a nested custom rule better and we can repurpose this issue. |
If we look at this from other perspective, this feature eases the markdownlint custom rule creation; non-developers can define their rules easily by simply editing the config file. So this is not a custom rule but a custom rule creation feature. Implementing the feature directly in markdownlint will ensure those custom rules provide same functionality as normal MD### rules do. If we want to keep it separate then can we mention it in markdownlint/README.md file? Because people may find coding custom rules difficult for various reasons.
For context behind this refer mdn/content#21432 (comment). Following issues need to be addressed to support inline enable/disable comments(lets call it inline configurations):
Re-implementing the entire feature in custom rules is not feasible in long term, we'll have to keep synchronizing the feature with the markdownlint repo. So we shouldn't address the issue no. 2 mentioned above. I think following provisions from markdownlint repo are required: b. Pass the configuration object to custom rules. More simplifications to the object are welcomed:
c. The helpers.js need to provide a function to convert the config object(b.) to ranges object like existing |
I am happy to include a link to this rule from the documentation! If you'd like to send a PR, I expect to publish a new release soon, and it could be part of that. Regarding your suggestions around passing more context to rules when disabling them, you make good points. I don't want to rush into that, so I'm not going to make it part of this release, but I have added a note to myself to think about it for the following release. Thanks again for the contribution, I really like what you've done to make custom rules more approachable! |
BTW, I realized afterwards, that this could also replace something like MD044 https://github.com/DavidAnson/markdownlint/blob/main/doc/Rules.md#md044---proper-names-should-have-the-correct-capitalization |
The “selectively disable part of a rule” proposal feels weird and special-case to me. I think a more general approach might be to allow reconfiguring a rule inline, but that would be a significant change. Since yours is the only scenario I recall where this has come up so far, what about an alternative approach? You could re-export the markdownlint-rule-search-replace rule multiple times under different names and then configure them with the different sets of strings and disable the set you want by rule name in the usual manner. If it works, this should be possible today. To be clearer, I’m imagining something like this (from memory on my phone):
|
In short, the custom rule only needs disabled line ranges and names of the disabled sub-rules.
Are you suggesting to write a new custom rule in the target repo(say Is it possible to export multiple rule definitions from a single custom rule file, i.e. |
I am proposing a new "meta-rule" which exports an array of multiple instances of the existing rule like you show (already supported today). Each name would need to be unique, but the implementation should all be the same, so the pattern I show should be all the extra "code" that's needed. If you specify configuration via JSON, it would need to be duplicated for each unique name. If you specify configuration via JS, you could reuse a common base object for each instance. You shouldn't have to do anything to the CLI and this should work with all the tools, I think. |
Regarding splitting configuration, it looks like you have about eight different definitions in the file you link to. In the extreme, you could give each one its own instance of the sub-rule, therefore exporting eight copies from the meta-rule. Or you could just break off one or two of those that need to be disabled inline so your meta-rule would only have two or three exports. |
Actually, the name "meta-rule" is a bad choice on my part. A single file can export multiple rules in an array. That's all I'm suggesting. Each one of those would have a new name but would otherwise be exactly the definition your rule has today. |
I think we don't have to split the sub-rules in the config file. We can simply, in the "meta-rule", intercept the sub-rule's // search-replace-spllitter.js (aka meta-rule)
const rule = require(“markdownlint-rule-search-replace”);
const config = readConfig(".markdownlint-cli2.jsonc");
const subRules = [];
for (const subRuleConfig in config["search-replace"].rules) {
const newRule = {
...rule,
"names": [subRuleConfig.name]
};
const originalFunction = rule.function.bind(newRule);
newRule.function = (params, onError) => { // wrap the "function" definition
// and tweak the "params" to have only the required rule config
params.config.rules = [subRuleConfig];
return originalFunction(params, onError);
};
subRules.add(newRule);
}
module.exports = subRules; This way the config file will remain the same, non weird, for the normal contributors. Edit: updated the function binding location. |
Smart!!! (Just curious, why do you need to bind vs. calling rule.function with the modified params directly? |
In case the custom rule uses |
As part of the Markdownlint cleanup on mdn/content, @OnkarRuikar made this interesting rule https://github.com/OnkarRuikar/markdownlint-rule-search-replace
This is a very useful swiss army knife rule that allows projects to identify (and optionally fix) issues specific to their code base.
One of the limitations that might be able to be addressed if move here, is that the entire ruleset needs to be disabled if you want to disable a single rule in a file. I believe that the limitation currently is that the disable syntax doesn't support extra metadata like
mardownlint-disable search-replace:my-sub-rule1
Maybe if integrating it doesn't make sense, then I could spin up a separate issue around that metadata disable part
The text was updated successfully, but these errors were encountered: