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 --fix for sort-prop-types #2505

Merged
merged 1 commit into from Nov 28, 2019

Conversation

webOS101
Copy link
Contributor

This addresses #1940 by disabling the fix option for sort-prop-types until a better fix to copy the comments is available.

@webOS101
Copy link
Contributor Author

webOS101 commented Nov 23, 2019

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 typescript-eslint-parser? Perhaps the dev dependency should be updated explicitly to a more recent version of eslint if it is required for testing? After I forced the install of eslint@6 linting seemed possible, though full of warnings.

Copy link
Member

@ljharb ljharb left a 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) {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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: [
Copy link
Member

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 outputs - since they're all correct for when we re-enable the fixer.

@webOS101
Copy link
Contributor Author

webOS101 commented Nov 23, 2019

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 jsx-sort-default-props also uses the same code and probably suffers the same problem, I didn't test it though. I threw in a test locally and it does have the same issue. After some direction I'll either redo this with jsx-sort-default-props unfixed as well or make another. Shame it never got renamed to sort-default-props.

@webOS101
Copy link
Contributor Author

@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 --fix for jsx-sort-default-props but think I should probably do that as well in this same PR. Agree?

@ljharb
Copy link
Member

ljharb commented Nov 25, 2019

Yes, I agree. Thanks!

@webOS101
Copy link
Contributor Author

@ljharb Disabled fix for jsx-sort-default-props. I think this is more-or-less ready to go.

@ljharb
Copy link
Member

ljharb commented Nov 28, 2019

Looks like there's some tests failing on master :-/ I filed eslint/eslint#12614 to track it.

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

2 participants