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

Disable fixing files if fixing can cause bugs #4573

Merged
merged 3 commits into from Feb 9, 2020
Merged

Disable fixing files if fixing can cause bugs #4573

merged 3 commits into from Feb 9, 2020

Conversation

bartlangelaan
Copy link
Member

@bartlangelaan bartlangelaan commented Jan 29, 2020

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

Currently, it's related to the pinned issues #4119 and #2643 - but more issues can be added with the same logic.

Is there anything in the PR that needs further explanation?

Yes.

Stylelint is an awesome tool, and I want to configure it for all projects we use at work. Because of the fixer, this would create almost no extra work for the team, because it can be run before somebody makes a commit.

There are however a few bugs open that make the autofixer behave unpredicted. I think the most known ones are #4119 and #2643. Looking at the comments in these issues, it looks like finding a solution for these issues is not easy.

I propose disabling the fixer when it detects that one of these issues may change the code in an unpredicted way. This way, the fixer doesn't always work, but it also never changes the code in an unpredicted way.

I added some code that tries to detect these bugs, and then disables the autofixer. I am unfamiliar with the code base of Stylelint, so bear with me 😄

There are probably better ways of coding this - if someone can can give me clues on what the 'right way' is, I'm happy to change my MR.

@jeddy3
Copy link
Member

jeddy3 commented Jan 29, 2020

@bartlangelaan Thank you! I like this proposal a lot. As you said, it makes stylelint predictable and useable while we work to address the underlying issues, which are non-trivial.

We are aware that #4119 and #2643 are pressing issues, and we've started to drum up support for fixing them. So far, we've:

This week I plan to:

  • fork the unmaintained parsers into the stylelint organisation
  • create an umbrella issue for those issues associated with this parser

(See #4560 (comment))

We hope that the umbrella issue will become something the community rallies around as we need help supporting the unmaintained CSS-in-JS parser.

Neither fixing the parser issues nor the disable comments will be easy tasks. For example,
the proposal in #2643 (comment) to fix disable comments sounds good to me, but I suspect it'll be a breaking change that will require the adapting of all 81 autofixable rules. We should do this, but it may take some time. That is why I like your pull request as it improves stylelint in the interim.

It'd be great to see this proposal grow to address issues like #4560, but as you said #4119 and #2643 seem to be most common. What does everyone think about trialling this proposal as-is in the next patch release and then incorporating more issues afterwards?

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

That's a great idea! Thank you for coming up with it!

Only two requests to make code more understandable.

lib/lintSource.js Outdated Show resolved Hide resolved
lib/lintSource.js Outdated Show resolved Hide resolved
@jeddy3
Copy link
Member

jeddy3 commented Feb 7, 2020

Once this goes in, we'll need to create a follow-up pull request to #4561 that document this approach within the:

The former section also seems like the best place to list the syntax patterns that cause the autofixer to ignore file by this workaround.

I can make the follow-up pull request once we merge this and #4561.


I also suggest we don't close any issues. We should add a comment to an issue when we add a workaround and state that the underlying problem still needs fixing instead.

@jeddy3 jeddy3 mentioned this pull request Feb 8, 2020
21 tasks
@bartlangelaan
Copy link
Member Author

Hey @hudochenkov, I've rewritten the code that was unclear. I hope it's clearer now, please let me know if it's not.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Good job! Thank you!

@hudochenkov
Copy link
Member

I also suggest we don't close any issues. We should add a comment to an issue when we add a workaround and state that the underlying problem still needs fixing instead.

Agree.

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Excellent stuff thank you!

I will create a follow-up pull request to document this feature once we merge #4587.

@jeddy3 jeddy3 merged commit 9f76ed3 into stylelint:master Feb 9, 2020
@jeddy3
Copy link
Member

jeddy3 commented Feb 9, 2020

Changelog message added:

  • Fixed: autofix will ignore sources containing disable comments or nested tagged template literals - this is workaround to make autofix safer to use until we can resolve the underlying issues (#4573).

@jeddy3
Copy link
Member

jeddy3 commented Feb 15, 2020

Released in 13.2.0.

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

3 participants