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 the ability to set a custom URL that is displayed in the error #108

Closed
ACyphus opened this issue Jun 19, 2023 · 9 comments · Fixed by #112
Closed

Add the ability to set a custom URL that is displayed in the error #108

ACyphus opened this issue Jun 19, 2023 · 9 comments · Fixed by #112

Comments

@ACyphus
Copy link

ACyphus commented Jun 19, 2023

This is a feature request to be able to define a custom URL for each rule in the config that is used when that rule shows an error.

Currently each error shows the URL of this repository:

https://github.com/OnkarRuikar/markdownlint-rule-search-replace/blob/main/rule.js#L139-L140

For my use case, the error is seen by people in GitHub pull requests that are not familiar with how markdownlint works. So when they are reading the error, a link to this repository doesn't help them.

Instead, if I was able to add custom URLs to either documentation or the rule definition in my config, they would be able to better understand the rule and how to fix the error.

Thanks for reading and your consideration! This is a helpful tool—I appreciate you working on it 💖

@OnkarRuikar
Copy link
Owner

The markdownlint package treats the entire custom rule as one rule, at the moment there is not way to make it recognize sub rules as individual. When the custom rule reports an error, this is how the information URL is being used in upstream markdownlint package:

      results.push({
        lineNumber,
        "ruleName": rule.names[0],
        "ruleNames": rule.names,
        "ruleDescription": rule.description,
        "ruleInformation": rule.information ? rule.information.href : null,
        "errorDetail": errorInfo.detail || null,
        "errorContext": errorInfo.context || null,
        "errorRange": errorInfo.range ? [ ...errorInfo.range ] : null,
        "fixInfo": fixInfo ? cleanFixInfo : null
      });

The URL is taken from the rule object and not errorInfo object. It would be ideal if markdownlint looks for the info URL in errorInfo object first.

@DavidAnson This is a valid request as any custom rule may want to provide info URL depending on the error. In order to implement this feature, is it possible to make following change?

-- "ruleInformation": rule.information ? rule.information.href : null,
++ "ruleInformation": errorInfo?.information?.href || rule?.information?.href || null,

@ACyphus till markdownlint supports this we can do either of these:
a. Provide info URL in custom rule's message itself. But this won't look good.
b. Provide entire rule wide custom info URL.

"search-replace": {
      "infoURL": "https://github.com/my/repo/README.md#all_custom_rules
      "rules": []
}

c. For now, to support URL for each rule we'll have to cheat a bit by setting href string on rule.information object before raising error every time. Need to check the feasibility.

Note: b. and c. will break if upstream implementation changes. markdownlint need to let custom rules change info URL mid run.

@DavidAnson
Copy link

I'm on my phone right now and only had a chance to skim this issue. I'm not sure I understand the issue, though. Each rule is meant to do a single thing and the URL is meant to explain that. If you are implementing multiple rules, you can export them all from the same package and each can have own URL. If it's confusing to point that URL to wherever it is today, you can point it at the markdownlint project if that's more useful? Basically, I'm not clear on why the URL for a particular rule should need to change dynamically. If you're able to clear that up, I would appreciate it. Thanks!

@DavidAnson
Copy link

Actually, I think I understand after looking at your project README. This seems like a bit of an edge case, but it's reasonable for me to add. I put it on my TODO list.

@DavidAnson
Copy link

This scenario will be supported in the next release of markdownlint: DavidAnson/markdownlint@c699b8e

@DavidAnson
Copy link

... and your sneaky trick of changing the rule definition dynamically will not: DavidAnson/markdownlint@14a7529

😀

@OnkarRuikar
Copy link
Owner

... and your sneaky trick of changing the rule definition dynamically will not: DavidAnson/markdownlint@14a7529

grinning

I haven't implemented the trick ;)
By the way this makes the tool more secure. We need to mention in the documentation that custom rules can not mutate.

@DavidAnson
Copy link

I feel like not mutating should be expected. My second change was just closing an unintentional hole.

@OnkarRuikar
Copy link
Owner

@ACyphus the feature has been published on NPM in the latest version v1.2.0 let me know if it solves your requirement.

@ACyphus
Copy link
Author

ACyphus commented Aug 23, 2023

Beautiful! It exactly solves my requirement—thank you @OnkarRuikar and @DavidAnson 💖 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants