Improved error messages for order-in rules
#23
Conversation
| return 'service'; | ||
| } | ||
|
|
||
| if (parentType === 'component') { |
There was a problem hiding this comment.
mostly style preference, but why not
if(parentType === 'component' && ember.isComponentLifecycleHook(node)) {
...
}There was a problem hiding this comment.
mostly just for consistency with the model condition below and the fact that this will be easier to extend should we want to add a second condition that only applies to components
| } | ||
|
|
||
| function getOrder(ORDER, type) { | ||
| for (var i = 0, len = ORDER.length; i < len; i++) { |
There was a problem hiding this comment.
since we're using ES6 features in some places already, it would be cleaner IMO to use forEach - could just as well be addressed as a bulk change in later PR's though
There was a problem hiding this comment.
you can't use forEach here because there is no way to break out of a forEach before having iterated over the whole array. in this case though we can stop iterating once we've found what we're looking for.
| //------------------------------------------------------------------------------ | ||
| // Organizing - Organize your components and keep order in objects | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| module.exports = function(context) { | ||
| var options = context.options[0] || {}; | ||
| var order = options.order || ORDER; |
There was a problem hiding this comment.
Nice! Good idea with giving developers option to change default order 👍
|
I'm really happy with these changes @Turbo87 :) I think that we should address ES6 changes later @jbandura, and like you say - bulk update whole project. One thing that is missing here is documentations for those rules. We should add information about the possibility of changing default order. Other than that truly good job! 🚀 |
|
@michalsnik I'd say lets test this out first as an experimental option and document it for the release after that. just in case something is not yet working as expected and we'd like to change some behavior. |
|
Alright, makes sense. |
|
@michalsnik is a new release going to be cut for this merge? |
|
Yes, however I wanted to test it before releasing on master. I'll try to do this today. Unfortunately I didn't release beta if that's what you're asking too. |
|
No worries, was just curious. I can also help test the master version |
|
Thanks! Let me know if you find something :) |
|
The only issue I am seeing is that the order states that services should come before inherited props, when i do that, the |
|
@davidpett I think that issue already existed before my PR. I'm not sure how to solve this properly... |
|
This indeed seems to be an issue that existed before as well.. Regarding current order rules I think the best way would be to just set queryParams as the next thing after service injections and get rid of |
|
Released https://github.com/netguru/eslint-plugin-ember/releases/tag/v2.2.0 🎉 Great work @Turbo87 |
This PR implements improved error messages for
order-inrules and also makes those rules configurable if someone would like to use a different order.Example:
The "currentUser" service injection should be above the inherited "queryParams" property on line 2/cc @michalsnik