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 --fix for sort-prop-types #2505
Conversation
I'd hoped to keep this PR simple, but it required removing quite a bit of code. Also, there are some failing Travis tests that seem unrelated to this PR. Finally, not sure why, but I couldn't seem to execute tests locally. Suspect it might be outdated eslint pulled in from |
1748d42
to
333ab65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd also be great to have a failing test (with commented-out output) that includes comments?
@@ -98,18 +95,6 @@ module.exports = { | |||
return; | |||
} | |||
|
|||
function fix(fixer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we keep this as commented code, and do something like const fix = undefined;
or something, so that all the rest of the code passing fix
doesn't have to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it might be easier to make this easy to revert to restore the missing code when desired. I tend to not like to keep large swaths of commented out code around, but I can do that if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally i'd agree, but in this case i think it'd be preferable.
@@ -486,18 +486,7 @@ ruleTester.run('sort-prop-types', rule, { | |||
line: 4, | |||
column: 5, | |||
type: 'Property' | |||
}], | |||
output: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly i wonder if we could comment out instead of remove all these output
s - since they're all correct for when we re-enable the fixer.
I can add the failing test case, commented out. As mentioned, I was keeping this easy to revert. I can either add the failing test here or in another PR, or I if you do want me to go the comment-out route, I can easily add the test in. I just remembered that |
@ljharb I went ahead and changed the removals to comments and added failing (but commented out) tests for sort with comments. I did not yet disable |
Yes, I agree. Thanks! |
@ljharb Disabled fix for |
3fd20a2
to
78d2ab6
Compare
Looks like there's some tests failing on master :-/ I filed eslint/eslint#12614 to track it. |
78d2ab6
to
55b605f
Compare
This addresses #1940 by disabling the fix option for
sort-prop-types
until a better fix to copy the comments is available.