stylelint icon indicating copy to clipboard operation
stylelint copied to clipboard

Add `url` secondary option to any rules

Open emmacharp opened this issue 1 year ago • 17 comments

What is the problem you're trying to solve?

Some generic rules (e.g. rule-selector-property-disallowed-list) can be used multiple times, for different reasons, but can link to only one URL. As it is, rule authors cannot point to the relevant documentation for each different use of the rule.

What solution would you like to see?

Enabling the use of custom URLs in the config file, like with messages.


  • [ ] rule-selector-property-disallowed-list
  • [ ] declaration-property-value-allowed-list

emmacharp avatar Jan 21 '24 11:01 emmacharp

@emmacharp Thanks for the proposal. Sounds interesting to me.

Like the message secondary option, a new option (e.g. url) seems to be as follows:

const config = {
  rules: {
    'custom-property-pattern': [/^[a-z]+$/, {
      url: 'https://your.custom.rule/resource', // as a string
    }],
    'color-no-hex': [true, {
      url: (rule) => `https://your.custom.rule/${rule}`, // as a function
    }]
  }
};

Does the example above satisfy your requirements?

ybiquitous avatar Jan 21 '24 12:01 ybiquitous

Yes, it seems like it does!

Thanks a bunch for the quick response. Looking forward to put that into action!

emmacharp avatar Jan 21 '24 13:01 emmacharp

Got it. Let's wait for other member's feedback.

ybiquitous avatar Jan 21 '24 13:01 ybiquitous

There seem no objections, so let's transition to "ready to implement".

Rule URLs are used only by a few formatters (for reporting). We can change the logic. E.g.

https://github.com/stylelint/stylelint/blob/5e2eff74a474af5fb0bdfd4c678f3e17bb758935/lib/formatters/verboseFormatter.mjs#L142-L148

if (typeof ruleConfig.secondaryOptions.url === 'function') {
  // `ruleConfig` can be retrieved from `returnValue.results[0]?._postcssResult?.stylelint.config?.rules`
  const url = ruleConfig.secondaryOptions.url({ name: rule, config: ruleConfig, message: reportedRuleMessage, ... });
  // ...
}

ybiquitous avatar Jan 31 '24 00:01 ybiquitous

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

github-actions[bot] avatar Mar 01 '24 00:03 github-actions[bot]

@ybiquitous, I'm having a try at implementing this and can't seem to get anything in the results array you specified above...

Maybe I'm being dense but if a try and log it in the verbose formatter, it always return undefined. Am I using it wrong?

Thanks for the help!

emmacharp avatar Mar 27 '24 15:03 emmacharp

@emmacharp Can you share your branch?

ybiquitous avatar Mar 27 '24 15:03 ybiquitous

You are fast, @ybiquitous! Hehe. Sorry for the delay. I didn't commit anything since I was only experimenting.

I tried this:

function ruleLink(rule, metadata, returnValue) {
	console.log(returnValue.results[0]);

	if (metadata && metadata.url) {
		return terminalLink(rule, metadata.url);
	}

	return rule;
}

and then added returnValue in verboseFormatter function:

output += dim(`  ${ruleLink(rule, meta, returnValue)}: ${list.length}${additional}\n`);

I also tried logging directly in the verboseFormatter function with the same undefined result.

If this message is not clear I can push this code to my fork and share it...

emmacharp avatar Mar 27 '24 18:03 emmacharp

Ah, I understand my explanation is not enough. 😅

Probably, we may have to save url properties in secondary options like message or severity properties.

https://github.com/stylelint/stylelint/blob/a5965a09b557fd4a5420a6b48f1c1353ecf83aa2/lib/lintPostcssResult.mjs#L102-L105

Users cannot change a rule metadata.

ybiquitous avatar Mar 28 '24 04:03 ybiquitous

I see. I'll have a look there. But maybe it's above my pay grade. hehe.

But then, about my previous question, shouldn't I be able to access returnValue.results[0]in the verboseFormatter function?

For instance, ant the bottom of the function like this:

        ...
	console.log(returnValue.results[0]);
	return `${output}\n`;
}

Sorry for the inconvenience. If I can't get it to work, I'll leave this to professionals and leave you be!

emmacharp avatar Mar 28 '24 10:03 emmacharp

@emmacharp Ah, I understand what you mean in the questions above. When a formatter function is called, lint results are not yet set to returnValue.results. See the code:

https://github.com/stylelint/stylelint/blob/a5965a09b557fd4a5420a6b48f1c1353ecf83aa2/lib/prepareReturnValue.mjs#L67-L69

I forgot why, but I guess we cannot easily change the formatter function signature for backward compatibility. Formatter authors can use the first argument of a formatter function to access lint problems reported.

ybiquitous avatar Mar 28 '24 12:03 ybiquitous

Thanks for heads up, @ybiquitous! I'll see if I can make progress and I'll report back. Any pointers as to where/how I should try and implement this (if you think of anything) will be welcome.

emmacharp avatar Mar 28 '24 13:03 emmacharp

Reporting back, @ybiquitous !

Here are the changes (in progress) made: 301bec8. I have two questions as of now:

  1. I can access the url through the rules property mentioned above (not sure I did right though) but I wonder if url should be treated as the message in the Warning object. Any thoughts?
  2. To make it work, I had to add url in the possible options in a rule... but if a custom url is possible on any rule, I guess there would be a better way to go about it?

Not sure it's done right but hope it is. Hehe. Thanks!

emmacharp avatar Mar 29 '24 15:03 emmacharp

@emmacharp The commit looks good. Can you open a PR if you're ready?

Rather than the message property, adding a new url property to the Warning object is better. E.g.

{
  "line": 1,
  "column": 1,
  "endLine": 10,
  "endColumn": 2,
  "rule": "unit-disallowed-list"
  "severity": "error",
  "text": "Unexpected unit \"px\" (unit-disallowed-list)"
+ "url": "https://stylelint.io/user-guide/rules/declaration-property-unit-disallowed-list"
}

This addition will benefit for the string and json formatters, too.

ybiquitous avatar Mar 29 '24 23:03 ybiquitous

@ybiquitous, yes that's what I thought about.

But I'm not sure how/where to set the value of a url property in the Warning object from the config . Can you give me a pointer?

And do we need to add the url secondary option in every rule or is there a more efficient way to do it (like the custom messages, for instance) ?

I'll gladly open a PR afterwards!

emmacharp avatar Mar 30 '24 12:03 emmacharp

@emmacharp You can set url to warningProperties as customMessages:

https://github.com/stylelint/stylelint/blob/a5965a09b557fd4a5420a6b48f1c1353ecf83aa2/lib/utils/report.mjs#L119-L125

ybiquitous avatar Mar 30 '24 13:03 ybiquitous

Super, I'll have a look at this. Thanks!

emmacharp avatar Mar 30 '24 13:03 emmacharp

Hi @ybiquitous !

It's been a while but I've come back to this and I think I understand what's going on here. Still, I wonder: customMessagesand the result of warn() are strings. If I want to include an url there, I'll have to change the type to an array, for instance, changing the definition of warn(). It seems to be quite risky, is it not?

I don't see how to do it another way but maybe I'm mistaken.

Would be glad to have your thoughts on this!

emmacharp avatar Jun 04 '24 11:06 emmacharp

@emmacharp It'd be great if you could come back! My explanation might be wrong, so let me explain again. My rough idea on this feature is as follows:

  • Add customUrls (like customMessages) to StylelintPostcssResult. This will store url secondary option values in users' configuration.
    • type StylelintPostcssResult = {
        customUrls: { [ruleName: string]: string }; // to keep it simple, only string is given.
      };
      
    • https://github.com/stylelint/stylelint/blob/e3615618321833fe4b0b57c065416da977c87def/types/stylelint/index.d.ts#L156-L159
    • https://github.com/stylelint/stylelint/blob/e3615618321833fe4b0b57c065416da977c87def/lib/lintPostcssResult.mjs#L100-L103
  • Add url? to WarningOptions.
    • type WarningOptions = {
        url?: string;
      };
      
    • https://github.com/stylelint/stylelint/blob/e3615618321833fe4b0b57c065416da977c87def/types/stylelint/index.d.ts#L171-L175
  • Add url? to Warning.
    • type Warning = {
        url?: string;
      };
      
    • https://github.com/stylelint/stylelint/blob/e3615618321833fe4b0b57c065416da977c87def/types/stylelint/index.d.ts#L480-L501
  • In report(), set a custom URL to warn() as a warning option.
    • const { customUrls } = result.stylelint;
      if (customUrls.[ruleName]) {
        warningProperties.url = customUrls[ruleName];
      }
      result.warn(warningMessage, warningProperties);
      
    • https://github.com/stylelint/stylelint/blob/e3615618321833fe4b0b57c065416da977c87def/lib/utils/report.mjs#L115-L121
  • In createPartialStylelintResult(), copy a URL from warnings to messages.
    • https://github.com/stylelint/stylelint/blob/e3615618321833fe4b0b57c065416da977c87def/lib/createPartialStylelintResult.mjs#L64

Please feel free to comment if you still have any questions. 😃

ybiquitous avatar Jun 04 '24 14:06 ybiquitous

Oh wow. You pretty much did the work for me... I'm kind of sorry if you felt you had to. Heh.

I'll look into this later in the day. Thank you for the support, as always!

emmacharp avatar Jun 05 '24 11:06 emmacharp

It works!

I must admit to being confused by "messages" which seems to mean two different things if I'm not mistaken (first the warning message as a whole and then the text message inside of it)?

Now, I think I may have to add the url option to every rule, is that right? Or is there a more efficient way?

I'll make a pull request afterwards. Thanks, you're a boss!

emmacharp avatar Jun 05 '24 19:06 emmacharp

Myimplementation idea would add the support for the url option, but there seem to be no problem. 👍

ybiquitous avatar Jun 06 '24 08:06 ybiquitous

Ok, just to make sure: your implementation does not "implicitly" support an url option, right?

I have to add url to validOptions in every rule, like this:

{
	actual: secondaryOptions,
	possible: {
		ignore: ['comments'],
		url: [isString],
	},
	optional: true,
}

It does not seem to work without it.

emmacharp avatar Jun 06 '24 09:06 emmacharp

No, we don't have to check url in every rule. 🤔

If you are ready for PR, I recommend opening it. We can discuss code on that PR in details.

ybiquitous avatar Jun 06 '24 11:06 ybiquitous

Done! #7743

emmacharp avatar Jun 06 '24 13:06 emmacharp