-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Teach sort-comp rule about getters and setters. #1311
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
Conversation
This is a phenomenal pull request. Phenomenal. |
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.
Thanks, this looks great! A few comments; plus it'll need tests, but then this should be a great semver-minor.
lib/rules/sort-comp.js
Outdated
@@ -136,6 +136,24 @@ module.exports = { | |||
} | |||
} | |||
|
|||
if (method.getter) { | |||
for (i = 0, j = methodsOrder.length; i < j; i++) { |
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.
var getterIndex = methodsOrder.indexOf('getters');
if (getterIndex > 0) { indexes.push(getterIndex); }
lib/rules/sort-comp.js
Outdated
} | ||
|
||
if (method.setter) { | ||
for (i = 0, j = methodsOrder.length; i < j; i++) { |
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.
var setterIndex = methodsOrder.indexOf('setters');
if (setterIndex > 0) { indexes.push(setterIndex); }
Updated! |
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.
LGTM, well done!
break; | ||
} | ||
const staticIndex = methodsOrder.indexOf('static-methods'); | ||
if (staticIndex >= 0) { |
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.
Thanks for cleaning these others up too :-) I didn't realize you were following an existing pattern.
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.
No problem! Easy change.
Sorry for the inconvenience; I've just merged a refactor to start using arrow functions. Would you mind rebasing? |
No worries, rebased! |
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.
LGTM
We wanted to be able to sort our getters and setters and it was mentioned in an issue, so I implemented it. This PR allows getters and setters to be sorted by adding the keywords
getters
and/orsetters
to thesort-comp
rule.Fixes #100