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 disableFix secondary option #5460

Merged
merged 10 commits into from Sep 16, 2021
Merged

Add disableFix secondary option #5460

merged 10 commits into from Sep 16, 2021

Conversation

kuangrs
Copy link
Contributor

@kuangrs kuangrs commented Aug 12, 2021

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

Closes #5431

Is there anything in the PR that needs further explanation?

no

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.

@kuangrs Thanks for the pull request.

Can you add a test or two to standalone-fix.test.js, e.g. showing:

  • one rule being disabled, e.g. indentation
  • two rules being disabled
  • one rule being disabled and another still autofixing

docs/user-guide/configure.md Outdated Show resolved Hide resolved
@kuangrs
Copy link
Contributor Author

kuangrs commented Aug 17, 2021

@jeddy3 it's ok, pls review

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.

Thanks for adding tests.

I've requested we tweak the tests a bit to focus more on the CSS rather than the violations.

fix: true,
});

expect(JSON.parse(result.output)[0].errored).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

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

Rather than check for errors, can we instead check if the output itself has been changed (like the tests at the beginning of the file), e.g.:

const code = "  a { color: red; }";

const result = await standalone({
    code,
    config: {
        rules: {
            indentation: [
                2,
                {
                    disableFix: true,
                },
            ],
        },
    },
    fix: true,
});

expect(result.output).toBe(code);

And:

const code = "  a { COLOR: red; }";

const result = await standalone({
    code,
    config: {
        rules: {
            indentation: [
                2,
                {
                    disableFix: true,
                },
            ],
            'property-case': ['lower'],
        },
    },
    fix: true,
});

expect(result.output).toBe("  a { color: red; }");

Copy link
Contributor Author

@kuangrs kuangrs Aug 19, 2021

Choose a reason for hiding this comment

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

@jeddy3 When we use disableFix, what we expect is that if there is an error in the check, the output still contains the error message instead of the initial code, so that we can find it easily.

Copy link
Member

Choose a reason for hiding this comment

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

Fair point.

@kuangrs kuangrs requested a review from jeddy3 August 20, 2021 06:23
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.

@kuangrs Sorry about this slipping off the radar.

LGTM.

@kuangrs kuangrs closed this Sep 15, 2021
@kuangrs kuangrs reopened this Sep 15, 2021
@kuangrs
Copy link
Contributor Author

kuangrs commented Sep 15, 2021

@jeddy3 thanks for your approval, Will it have a chance in Release14.0.0? hope my company's project can use this option earlier

@ybiquitous ybiquitous linked an issue Sep 15, 2021 that may be closed by this pull request
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

[suggestion] How about refactoring to use some() instead of filter()?

lib/__tests__/standalone-fix.test.js Outdated Show resolved Hide resolved
lib/__tests__/standalone-fix.test.js Outdated Show resolved Hide resolved
lib/__tests__/standalone-fix.test.js Outdated Show resolved Hide resolved
lib/__tests__/standalone-fix.test.js Outdated Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

It seems the added logic has no problems. 👍🏼
Thank you!

@jeddy3
Copy link
Member

jeddy3 commented Sep 15, 2021

Will it have a chance in Release14.0.0?

Yes, it'll make it into 14.0.0. However, there's still a lot to do before we can release 14.0.0. If you'd like to help us get it out of the door, please contribute to the outstanding tasks in:

kuangrs and others added 4 commits September 16, 2021 12:28
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@jeddy3
Copy link
Member

jeddy3 commented Sep 16, 2021

@kuangrs Can you run npm run format, please?

@kuangrs
Copy link
Contributor Author

kuangrs commented Sep 16, 2021

@kuangrs Can you run npm run format, please?

done

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM 👍🏼

@jeddy3 jeddy3 merged commit 6ab4199 into stylelint:v14 Sep 16, 2021
@jeddy3
Copy link
Member

jeddy3 commented Sep 16, 2021

  • Added: disableFix as secondary option to the rules property in the configuration object (#5460).

adrianjost added a commit to adrianjost/stylelint that referenced this pull request Dec 23, 2022
…ue` stylelint#6542

fixes stylelint#6542 and bv that should also fix stylelint/vscode-stylelint#369

Issue: The API did not return the fixed css in `output` if any rule contained `ruleDisableFix`.

I have removed the `ruleDisableFix` code, because it did not affect the code fixing itself and was assumingly wrongly used to change the content format of `output`.
I have adjusted some tests to adapt to the changed behaviour and extended the test conditions to check that the returned code has the correct fixes applied.

Issue was introduced in stylelint#5460
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.

Add disableFix secondary option to configuration object
4 participants