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 checkAgainstRule util #2173

Merged
merged 3 commits into from
Dec 12, 2016
Merged

Add checkAgainstRule util #2173

merged 3 commits into from
Dec 12, 2016

Conversation

davidtheclark
Copy link
Contributor

Here's an idea to solve #2139.

It exposes another function stylelint.checkAgainstRule, which you can use to run a PostCSS Root object through a rule, and invoke a callback with every warning that is created.

What I'd like to see in a situation liked #2139 is that the plugin rule that uses a core rule is the only one that calls report(). It is the reporting rule, and its severity should be used.

I have not yet added documentation, but did add some tests.

@hudochenkov would you please try this branch out and see if it works for your use-case?

@stylelint/core any input on this?

@ntwb
Copy link
Member

ntwb commented Dec 5, 2016

"... is that the plugin rule that uses a core rule is the only one that calls report(). It is the reporting rule, and its severity should be used."

SGTM 👍

@jeddy3
Copy link
Member

jeddy3 commented Dec 5, 2016

SGTM too.

The CI is failing because of a couple of flow warnings.

@hudochenkov
Copy link
Member

I don't know what callback to use in order to push warning where they belong, so I've used console.log for testing:

stylelint.checkAgainstRule(
	{
		ruleName: 'declaration-block-properties-order',
		ruleSettings: [ cleanedConfig, options ],
		root,
	},
	(warning) => console.log(warning)
);

I've used code above instead of https://github.com/hudochenkov/stylelint-order/blob/master/rules/declaration-block-property-groups-structure/index.js#L32-L40

It shows this warning on setup as in #2139:

Warning {
  type: 'warning',
  text: 'Expected "position" to come before "width" (declaration-block-properties-order)',
  line: 5,
  column: 4,
  severity: 'ignore',
  rule: 'declaration-block-properties-order',
  node:
   Declaration {
     raws: { before: '\n\n\t\t\t', between: ': ' },
     type: 'decl',
     parent:
      Rule {
        raws: [Object],
        type: 'rule',
        nodes: [Object],
        parent: [Object],
        source: [Object],
        selector: 'a',
        lastEach: 3,
        indexes: {} },
     source: { start: [Object], input: [Object], end: [Object] },
     prop: 'position',
     value: 'relative' } }

Problem with severity lays some where else :(

@davidtheclark
Copy link
Contributor Author

@hudochenkov Here's how you'd use this:

function myPluginRule(root, result) {
  /* ... */
  stylelint.checkAgainstRule({
    ruleName: 'declaration-block-properties-order',
    ruleSettings: someAppropriateSettings,
    root,
  }, (warning) => {
    stylelint.utils.report({
      result,
      ruleName: myPluginRuleName,
      message: myMessage,      
      node: warning.node,
      line: warning.line,
      column: warning.column
    })
  });
}

The idea is that you'd actually report a warning for your own rule. A user doesn't want to see a warning for declaration-block-properties-order if they didn't set that rule in their config — that would be confusing. They want to see the warning for myPluginRuleName, even if the logic that determined the warning originated in declaration-block-properties-order. Also, this way the severity set for myPluginRuleName will be reported with the warning.

Does this make sense?

@hudochenkov
Copy link
Member

Sounds reasonable. I've changed message: myMessage to message: warning.text, because I can't predict all the messages from core rule. So the output looks like this:

 5:4  ✖  Expected "position" to come before "width" (declaration-block-properties-order)  order/declaration-block-property-groups-structure

warning.text contains core rule name. Is it ok to show warning as above, or it's better to cut core rule name from warning.text?

@davidtheclark
Copy link
Contributor Author

Is it ok to show warning as above, or it's better to cut core rule name from warning.text

@hudochenkov If I were using that plugin, I think I'd find it confusing to see a rule mentioned that I did not deliberately turn on — so I'd suggest using a regex or something to replace the name with that of the rule that is reporting.

@jeddy3
Copy link
Member

jeddy3 commented Dec 10, 2016

It sounds like this approach is working out well.

I'll label this up as "needs docs".

@davidtheclark
Copy link
Contributor Author

Added some documentation in developer-guide/plugins.

@jeddy3
Copy link
Member

jeddy3 commented Dec 12, 2016

Excellent. That example really highlights the power of this addition!

@jeddy3
Copy link
Member

jeddy3 commented Dec 12, 2016

@davidtheclark Those flow errors have resurfaced. Should we fix those before merging?

@davidtheclark
Copy link
Contributor Author

Got the tests passing! Please take another look sometime :)

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

It looks very good 👍

@jeddy3 jeddy3 merged commit d28d139 into master Dec 12, 2016
@jeddy3 jeddy3 deleted the 2139-check-against-rule branch December 12, 2016 15:07
@jeddy3
Copy link
Member

jeddy3 commented Dec 12, 2016

Changelog

  • Added: stylelint.utils.checkAgainstRule for checking CSS against a standard stylelint rule within your own rule (#2173).

As an aside, I think with new features like this the next release is shaping up as a good one :)

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017
* Add checkAgainstRule util
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants