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

[Update] Make vue/order-in-components fixable #381

Merged

Conversation

ota-meshi
Copy link
Member

This PR makes vue/order-in-components fixable. #299
In case of The "A" property should be above the "B" property error, autofix will move A before B

This Commit makes `vue/order-in-components` fixable.
In case of `The "A" property should be above the "B" property` error, autofix will move A before B
@mysticatea
Copy link
Member

Thank you for the contribution! And I make apologies that I overlooked this PR. 🙇

Reordering needs to handle carefully because each property value can have side-effects. So ESLint core rules don't support reordering in autofix basically.

In this case, I think we can support reordering if it's sure those properties don't have side-effects.
Would you add the check whether all properties don't have side-effects into the fix() function? As details, all computed property keys and all property values don't contain CallExpression, NewExpression, UpdateExpression, AssignmentExpression, and TaggedTemplateExpression.

@ota-meshi
Copy link
Member Author

Thank you for checking.

I changed it to run fix() only when it's sure that there are no side effects.

In order to prevent side effects from future ES changes, it runs fix() only when all and each of the properties have node.type that do not cause side effects as far as I know, in them.

Although implementation has become complex, I think this is the only way to fully prevent troubles. However, it has become complex, so I would appreciate your feedback on whether to actually implement this change.

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Looks great!

I'm afraid a bit to use internal API of ESLint, but I'm OK since it's a long-living class.

};
`,
parserOptions,
output: `
Copy link
Member

Choose a reason for hiding this comment

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

Would you use output: null to specify "not fix" explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not know that. Thank you for the information.

@ota-meshi
Copy link
Member Author

Thank you for review!
I changed it again, so please check it.

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much!

@mysticatea mysticatea merged commit 6861c81 into vuejs:master Feb 17, 2018
@ota-meshi
Copy link
Member Author

I'm glad you approved!
I appreciate your quick response 😸

@ota-meshi ota-meshi deleted the develop/order-in-components--fixable branch February 19, 2018 16:10
michalsnik pushed a commit that referenced this pull request Feb 24, 2018
* [Update] Make `vue/order-in-components` fixable

This Commit makes `vue/order-in-components` fixable.
In case of `The "A" property should be above the "B" property` error, autofix will move A before B

* [fix] fail test at [email protected]

* [fix] If there is a possibility of a side effect, don't autofix

* [fix] failed test at node v4

* [update] use Traverser

* [fix] failed test [email protected]

* [fix] I used `output: null` to specify "not fix"
michalsnik pushed a commit that referenced this pull request Apr 1, 2018
* Add Vue.extend support, add missing info about Vue.mixin check in readme

* Docs: fixes wording in docs (#372)

* Fix: fix script-indent to prevent removing <script> tag (fixes #367) (#374)

*  [Update] Make `vue/max-attributes-per-line` fixable (#380)

* [Update] Make `vue/max-attributes-per-line` fixable

* [fix] bug and style

* [fix] Switch indent calculation method with node and attribute

* [fix] don't handle indentation

* [add] autofix test max-attributes-per-line.js

* Update: make `vue/order-in-components` fixable (#381)

* [Update] Make `vue/order-in-components` fixable

This Commit makes `vue/order-in-components` fixable.
In case of `The "A" property should be above the "B" property` error, autofix will move A before B

* [fix] fail test at [email protected]

* [fix] If there is a possibility of a side effect, don't autofix

* [fix] failed test at node v4

* [update] use Traverser

* [fix] failed test [email protected]

* [fix] I used `output: null` to specify "not fix"

* Fix: no-async-in-computed-properties crash (fixes #390)

* Fix: valid-v-on false positive (fixes #351)

* Fix: indent rules false positive (fixes #375)

* [New] Add `prop-name-casing`

* fix message and category

* fix category

* fix test message

* Set category to undefined

* Add more tests and fix edge case scenario

* attribute order linting

* updating docs, tests and category

* [Update] Make `vue/attributes-order` fixable

* [fix] That `vue/attributes-order` got duplicated when merging README

* [add] messed order properties test case

* [fix] unify input on test case
michalsnik pushed a commit that referenced this pull request Jul 11, 2018
* Add Vue.extend support, add missing info about Vue.mixin check in readme

* Docs: fixes wording in docs (#372)

* Fix: fix script-indent to prevent removing <script> tag (fixes #367) (#374)

*  [Update] Make `vue/max-attributes-per-line` fixable (#380)

* [Update] Make `vue/max-attributes-per-line` fixable

* [fix] bug and style

* [fix] Switch indent calculation method with node and attribute

* [fix] don't handle indentation

* [add] autofix test max-attributes-per-line.js

* Update: make `vue/order-in-components` fixable (#381)

* [Update] Make `vue/order-in-components` fixable

This Commit makes `vue/order-in-components` fixable.
In case of `The "A" property should be above the "B" property` error, autofix will move A before B

* [fix] fail test at [email protected]

* [fix] If there is a possibility of a side effect, don't autofix

* [fix] failed test at node v4

* [update] use Traverser

* [fix] failed test [email protected]

* [fix] I used `output: null` to specify "not fix"

* Fix: no-async-in-computed-properties crash (fixes #390)

* Fix: valid-v-on false positive (fixes #351)

* Fix: indent rules false positive (fixes #375)

* [New] Add `prop-name-casing`

* fix message and category

* fix category

* fix test message

* Set category to undefined

* Add more tests and fix edge case scenario

* attribute order linting

* updating docs, tests and category

*  [New] Add rule `vue/no-use-v-if-with-v-for` (#2)

* [fix] `meta.docs.description` should not end with `.`  consistent-docs-description

* [fix] lint error caused by merging the master for conflict resolution

* [fix] That `vue/attributes-order` got duplicated when merging README

* [fixed] document `correct` and `incorrect` the contrary stated

* [fixed] error message
@bbugh
Copy link

bbugh commented Jul 13, 2018

This doesn't seem to work when I've customized an extra property that I want to have automatically sorted. I notice a discussion about side effects. Is there a way to tell it to automatically fix "unapproved" properties as well? We want it in a specific order on purpose, not just as a warning. If I remove the item from the rule list (validations in this case), then everything sorts again.

@michalsnik
Copy link
Member

michalsnik commented Jul 16, 2018 via email

@bbugh
Copy link

bbugh commented Jul 16, 2018

Thank you for confirming that it is an issue, I will make a new issue.

michalsnik pushed a commit that referenced this pull request Jul 30, 2018
* Add Vue.extend support, add missing info about Vue.mixin check in readme

* Docs: fixes wording in docs (#372)

* Fix: fix script-indent to prevent removing <script> tag (fixes #367) (#374)

*  [Update] Make `vue/max-attributes-per-line` fixable (#380)

* [Update] Make `vue/max-attributes-per-line` fixable

* [fix] bug and style

* [fix] Switch indent calculation method with node and attribute

* [fix] don't handle indentation

* [add] autofix test max-attributes-per-line.js

* Update: make `vue/order-in-components` fixable (#381)

* [Update] Make `vue/order-in-components` fixable

This Commit makes `vue/order-in-components` fixable.
In case of `The "A" property should be above the "B" property` error, autofix will move A before B

* [fix] fail test at [email protected]

* [fix] If there is a possibility of a side effect, don't autofix

* [fix] failed test at node v4

* [update] use Traverser

* [fix] failed test [email protected]

* [fix] I used `output: null` to specify "not fix"

* [New] Add `vue/component-name-in-template-casing`

* [update] documents

* [fix] require-meta-docs-url

* [fix] failed tests

* [fix] review contents

* Default case to constant. and used it. `const defaultCase = 'PascalCase'`
* `'category'` to `undefined`
* Add empty line for clarity
* I used object shorthand. `caseType: caseType` to `caseType`

* [fix] No deletes space and attributes of endTag

* [fix] Remove test unnecessary option

* [fix] lint error caused by merging the master for conflict resolution

* Add ignores option.

* Fixed that extra differences.

* update docs link

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

Successfully merging this pull request may close these issues.

4 participants