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 url secondary option to any rules #7484

Open
2 tasks
emmacharp opened this issue Jan 21, 2024 · 17 comments
Open
2 tasks

Add url secondary option to any rules #7484

emmacharp opened this issue Jan 21, 2024 · 17 comments
Labels
status: ask to implement ask before implementing as may no longer be relevant type: enhancement a new feature that isn't related to rules

Comments

@emmacharp
Copy link
Contributor

emmacharp commented Jan 21, 2024

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
@ybiquitous ybiquitous added the status: needs discussion triage needs further discussion label Jan 21, 2024
@ybiquitous
Copy link
Member

@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?

@emmacharp
Copy link
Contributor Author

Yes, it seems like it does!

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

@ybiquitous
Copy link
Member

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

@ybiquitous
Copy link
Member

ybiquitous commented Jan 31, 2024

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.

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

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 ybiquitous added status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules and removed status: needs discussion triage needs further discussion labels Jan 31, 2024
@ybiquitous ybiquitous changed the title Custom URLs in config file Add url secondary option Jan 31, 2024
@ybiquitous ybiquitous changed the title Add url secondary option Add url secondary option to any rules Jan 31, 2024
Copy link
Contributor

github-actions bot commented Mar 1, 2024

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

@github-actions github-actions bot added status: ask to implement ask before implementing as may no longer be relevant and removed status: ready to implement is ready to be worked on by someone labels Mar 1, 2024
@emmacharp
Copy link
Contributor Author

@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!

@ybiquitous
Copy link
Member

@emmacharp Can you share your branch?

@emmacharp
Copy link
Contributor Author

emmacharp commented Mar 27, 2024

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...

@ybiquitous
Copy link
Member

ybiquitous commented Mar 28, 2024

Ah, I understand my explanation is not enough. 😅

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

postcssResult.stylelint.ruleSeverities[ruleName] =
(secondaryOptions && secondaryOptions.severity) || defaultSeverity;
postcssResult.stylelint.customMessages[ruleName] = secondaryOptions && secondaryOptions.message;
postcssResult.stylelint.ruleMetadata[ruleName] = ruleFunction.meta || {};

Users cannot change a rule metadata.

@emmacharp
Copy link
Contributor Author

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!

@ybiquitous
Copy link
Member

@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:

returnValue.report = formatter(stylelintResults, returnValue);
returnValue._output = returnValue.report; // TODO: Deprecated. Remove in the next major version.
returnValue.results = stylelintResults;

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.

@emmacharp
Copy link
Contributor Author

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
Copy link
Contributor Author

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!

@ybiquitous
Copy link
Member

@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.

@emmacharp
Copy link
Contributor Author

@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!

@ybiquitous
Copy link
Member

@emmacharp You can set url to warningProperties as customMessages:

const { customMessages } = result.stylelint;
const warningMessage = buildWarningMessage(
(customMessages && customMessages[ruleName]) || message,
messageArgs,
);
result.warn(warningMessage, warningProperties);

@emmacharp
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ask to implement ask before implementing as may no longer be relevant type: enhancement a new feature that isn't related to rules
Development

No branches or pull requests

2 participants