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

Warnings are logged multiple times on a single run #978

Closed
zspecza opened this issue Mar 25, 2016 · 6 comments
Closed

Warnings are logged multiple times on a single run #978

zspecza opened this issue Mar 25, 2016 · 6 comments

Comments

@zspecza
Copy link

zspecza commented Mar 25, 2016

Note:

Forgive me if I'm putting this in the wrong place - I'm unaware if this bug originates from stylelint or postcss-reporter.

Bug Report:

Describe the issue. Is it a bug or a feature request (new rule, new option, etc.)?

Linter warnings are reported multiple times for each file on a single run:

a screenshot of the problem

I have set postcss-reporter's clearMessages option to true, and tried false - the bug is consistent both times.

Which rule, if any, is this issue related to?

None.

What CSS is needed to reproduce this issue?

.foo, .bar // to trigger newline rule
  color: pink 

What stylelint configuration is needed to reproduce this issue?

{
  "extends": "stylelint-config-standard",
  "rules": {
    "declaration-block-trailing-semicolon": null
  }
}

Which version of stylelint are you using?

5.2.0

How are you running stylelint: CLI, PostCSS plugin, Node API?

Consumed as a PostCSS plugin in the asset pipeline for Roots. Example Roots config app.coffee:

stylelint = require 'stylelint'
css_pipeline = require 'css-pipeline'

module.exports =
  postcss:
    use: [stylelint]
  extensions:
    css_pipeline
      files: 'assets/css/*.css'

Does your issue relate to non-standard syntax (e.g. SCSS, nesting, etc.)?

Yes, syntax is SugarSS, with precss.

What did you expect to happen?

Warnings should have only been reported once.

What actually happened (e.g. what warnings or errors you are getting)?

Warnings were reported three times.

@zspecza
Copy link
Author

zspecza commented Mar 25, 2016

Removing postcss-reporter from the pipeline makes it only log 2 warnings - I think this may just be specific to this particular setup - Closing this issue.

@zspecza zspecza closed this as completed Mar 25, 2016
@jeddy3
Copy link
Member

jeddy3 commented Mar 27, 2016

@declandewet Just a note to say this is the first issue we've had concerning the new SugarSS syntax and stylelint. It's kind of fascinating to see. Can I ask a question or two about how SugarSS and stylelint are working out for you?

I see you're using stylelint-config-standard. Have you found it to be broadly compatible with SugarSS? I see you're turning off the "declaration-block-trailing-semicolon". That makes sense. If that's the only rule that's incompatible with SugarSS then we should consider removing it from stylelint-config-standard.

I see you're using stylelint as a PostCSS plugin (through Roots). Is that because using the PostCSS plugin is your preferred method, or because the SugarSS syntax isn't yet available when using the CLI or Node API? Would you use either of these the CLI or Node API if a SugarSS syntax option was added?

Thanks for your in-depth bug report btw. I'm glad you were able to resolve the issue yourself.

@zspecza
Copy link
Author

zspecza commented Mar 28, 2016

Have you found it [stylelint] to be broadly compatible with SugarSS?

Yes.

I see you're turning off the "declaration-block-trailing-semicolon". That makes sense. If that's the only rule that's incompatible with SugarSS then we should consider removing it from stylelint-config-standard.

So far, that's the only rule I've found to be incompatible with SugarSS. However, I don't think it should be removed from stylelint-config-standard - the SugarSS syntax isn't for everyone - and a lack of semicolons is invalid in regular css syntax. I think keeping things the way they are is fine - perhaps adding a note to the README regarding this rule and SugarSS is enough - I don't think the change is big enough to warrant a new standard config.

I see you're using stylelint as a PostCSS plugin (through Roots). Is that because using the PostCSS plugin is your preferred method, or because the SugarSS syntax isn't yet available when using the CLI or Node API?

The PostCSS way is the easiest to manage within the Roots stack, as Roots extensions and Accord (the transpiler engine powering Roots) are first-class citizens of the setup (if that makes sense). While it's possible to use the Node API, it would be rather awkward. That being said:

Would you use either of these the CLI or Node API if a SugarSS syntax option was added?

Personally, I would not - but I do have a hunch other people might, so it might be worth exploring.

@jeddy3
Copy link
Member

jeddy3 commented Mar 28, 2016

Thanks for taking the time out to reply. It's much appreciated.

and a lack of semicolons is invalid in regular css syntax

FYI, the declaration-block-trailing-semicolon rule checks the optional trailing semicolon at the end of a declaration block, rather than checking semicolons between declarations (which are a mandatory in standard CSS). Some styleguides enforce it and some don't for various reasons.

I don't think the change is big enough to warrant a new standard config.

Agreed. However, there's a breaking release of the config pending and so we might roll it into that. We've been adding more "catching subtle mistakes" rules (like shorthand property overrides) to the config recently while removing the divisive stylistic stuff. Thanks for your thoughts on the matter though and I'll reference them in the discussion issue over in the config repo.

I do have a hunch other people might, so it [SugarSS syntax option] might be worth exploring.

Agreed. I'll raise a separate issue for that.

@zspecza
Copy link
Author

zspecza commented Mar 28, 2016

FYI, the declaration-block-trailing-semicolon rule checks the optional trailing semicolon at the end of a declaration block

Didn't notice that. That's good to know. That changes my viewpoint slightly - which makes me agree with your second point.

Agreed. I'll raise a separate issue for that [supporting a SugarSS syntax option].

Perhaps this is where one should assume the YAGNI principle pending further discussion & agreement among users.

@jeddy3
Copy link
Member

jeddy3 commented Mar 28, 2016

Perhaps this is where one should assume the YAGNI principle pending

That sounds like a plan. It's unlikely that this feature will be added until someone comes along who needs it and sends a PR.

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

No branches or pull requests

2 participants