Report correct positions for "order-in-*" rules #15

Turbo87 · 2017-02-03T17:21:29Z

Resolves #13

Example:

export default Ember.Route.extend({
  foo: 'bar',
  foo2: 'bar',

  setupController() {},

  foo3: 'bar',

  actions: {
    bar() {}
  },

  model() {

  },

  foo4: 'bar',
});

app/routes/foo.js
   9:3   error  The property should be above the lifecycle hook on line 7    ember/order-in-routes
  15:3   error  The model hook should be above the lifecycle hook on line 7  ember/order-in-routes
  19:3   error  The property should be above the lifecycle hook on line 7    ember/order-in-routes

…erties

and derive "order" from that

... that need to be moved further up

…perties

michalsnik · 2017-02-04T16:46:29Z

lib/utils/property-order.js

+      var nextSourceLine = firstPropertyOfNextType.node.loc.start.line;
+
+      context.report(property,
+        `The ${typeName} should be above the ${nextTypeName} on line ${nextSourceLine}`);


What do you think about updating this to provide also information about the initial line number?

var sourceLine = info.node.loc.start.line; // ... `The ${typeName} at line ${sourceLine} should be above the ${nextTypeName} on line ${nextSourceLine}`);`

that's unnecessary because ESLint will add that information itself in the reporter

michalsnik · 2017-02-05T10:19:23Z

lib/rules/order-in-components.js

+];
+
+const NAMES = {
+  'service': 'service injection',


What do you think about moving all those names to a separate file and using them inside property-order.js? It looks like there are some unnecessary duplications in current solution.

yeah, we could probably do that in a separate refactoring. I would also like to add the names of the properties to the type descriptions if possible, but I wanted to get this PR merged first.

Ok, that's fine to me 👍

michalsnik · 2017-02-05T10:22:40Z

lib/rules/order-in-models.js

+  'relationship': 'relationship',
+  'single-line-function': 'single-line function',
+  'multi-line-function': 'multi-line function',
+  'other': 'property',


Maybe here we can also assume that otherwise there is an unknown instead of other?

the problem is that "unknown" is different than "other". For "unknown" we really don't know what type of property it could be because we should've caught all the ones we know about. For "other" we don't analyze all and treat all the ones we haven't analyzed further the same.

Okay, so let's leave this as a separate concern. Anyway your solution seems to be better than enough and I'm happy to push this forward :)

michalsnik

One more approve and we're ready to release new version 🚀

bardzusny · 2017-02-05T17:31:04Z

lib/rules/order-in-components.js

 var ember = require('../utils/ember');
+var reportUnorderedProperties = require('../utils/property-order').reportUnorderedProperties;
+
+const ORDER = [


We're using ES5 across the rest of the codebase. For now I'd revert to var, and start an effort which would:

Add lint config, ran as part of CI check - ensuring consistency here (linting ESLint plugin, inception :) ).

Update test matrix to run not only Node.js 6 (as currently - https://github.com/netguru/eslint-plugin-ember/blob/master/.travis.yml) but also oldest version that we're striving to support (likely 4).

I've just created related issues - see #17 , #18 .

bardzusny · 2017-02-05T17:31:30Z

lib/rules/order-in-components.js

+  'unknown',
+];
+
+const NAMES = {


See comment above about const/var.

bardzusny · 2017-02-05T17:32:12Z

lib/rules/order-in-components.js

+    return 'actions';
+  } else if (ember.isComponentCustomFunction(node)) {
+    return 'method';
+  } else {


This else is redundant - if we got that far, we can just return directly. I understand it's a matter of style preference, though.

should I change all the else if too just if too?

bardzusny · 2017-02-05T17:32:59Z

lib/rules/order-in-routes.js

+};
+
+function toType(node) {
+


Blank line.

bardzusny · 2017-02-05T17:34:55Z

tests/lib/rules/order-in-controllers.js

@@ -15,63 +15,114 @@ var eslintTester = new RuleTester();
 eslintTester.run('order-in-controllers', rule, {
  valid: [
    {
-      code: 'export default Controller.extend();',
+      code: `export default Controller.extend();`,


Template strings - see comment above about const/var, this is related.

template strings are supported in Node 4, shouldn't be a problem

bardzusny

Not requesting changes per se - but I think we should at the very least update CI config to consider oldest supported Node.js version before merging, since some ES6 features were used here (understandably - life is hard without them :) ). So let's wait with merge until we're 100% sure this won't cause any regressions - either by modifying Travis configuration, or resigning from const/template strings for now.

bardzusny

Little uncomfortable with basic ES6 constructs appearing in the codebase for the first time out of nowhere. With #17, #18 and #19 on the way though - I'm happy to approve. 🚢 🚀

Turbo87 added 19 commits February 3, 2017 18:19

tests/rules/order-in-components: Use template strings for "code" prop…

4c0fc24

…erties

rules/order-in-components: Extract ComponentPropertyInfo class

512630e

rules/order-in-components: Implement "type" property

fd063ae

and derive "order" from that

rules/order-in-components: Inline mappedProperties variable

7aa1a6f

tests/rules/order-in-components: Assert "line" properties

9a5b72f

rules/order-in-components: Report multiple properties

dc8f875

... that need to be moved further up

rules/order-in-components: Report correct positions

6b4341d

rules/order-in-components: Add "name" property

15de514

rules/order-in-components: Merge the two loops

ac6a36a

rules/order-in-components: Remove unused variable and function

29a08bf

rules/order-in-components: Convert "type" to local variable

ded22ee

rules/order-in-components: Extract reusable function

eaddfc8

rules/order-in-components: Move export to the bottom

f0011cf

tests/rules/order-in-controllers: Use template strings for "code" pro…

1a9494c

…perties

tests/rules/order-in-models: Use template strings for "code" properties

313cbc8

tests/rules/order-in-routes: Use template strings for "code" properties

c13f468

rules/order-in-controllers: Include correct position in warning

c332473

rules/order-in-models: Include correct position in warning

db3bf0e

rules/order-in-routes: Include correct position in warning

4fc0cce

Turbo87 changed the title ~~Report correct positions for "order-in-components" rule~~ Report correct positions for "order-in-*" rules Feb 3, 2017

Turbo87 mentioned this pull request Feb 4, 2017

Improve "order-in-*" rule warnings #13

Closed

michalsnik reviewed Feb 4, 2017

View reviewed changes

michalsnik reviewed Feb 5, 2017

View reviewed changes

michalsnik approved these changes Feb 5, 2017

View reviewed changes

bardzusny reviewed Feb 5, 2017

View reviewed changes

lib/rules/order-in-components.js

'unknown',

];

const NAMES = {

Copy link

Contributor

bardzusny Feb 5, 2017

Choose a reason for hiding this comment

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

See comment above about const/var.

bardzusny reviewed Feb 5, 2017

View reviewed changes

lib/rules/order-in-routes.js

};

function toType(node) {

Copy link

Contributor

bardzusny Feb 5, 2017

Choose a reason for hiding this comment

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

Blank line.

bardzusny reviewed Feb 5, 2017

View reviewed changes

bardzusny suggested changes Feb 5, 2017

View reviewed changes

bardzusny approved these changes Feb 5, 2017

View reviewed changes

michalsnik merged commit 9014cc3 into ember-cli:master Feb 5, 2017

michalsnik added the Done label Feb 5, 2017

Turbo87 deleted the order branch February 5, 2017 19:04

Turbo87 mentioned this pull request Apr 12, 2017

Could no-identical-names reference the line number of the previous test/module? qunitjs/eslint-plugin-qunit#62

Closed

Turbo87 added the enhancement label Nov 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report correct positions for "order-in-*" rules #15

Report correct positions for "order-in-*" rules #15

Turbo87 commented Feb 3, 2017 •

edited

Loading

michalsnik Feb 4, 2017

Turbo87 Feb 4, 2017

michalsnik Feb 5, 2017

Turbo87 Feb 5, 2017

michalsnik Feb 5, 2017

michalsnik Feb 5, 2017

Turbo87 Feb 5, 2017

michalsnik Feb 5, 2017

michalsnik left a comment

bardzusny Feb 5, 2017 •

edited

Loading

bardzusny Feb 5, 2017 •

edited

Loading

bardzusny Feb 5, 2017

bardzusny Feb 5, 2017

Turbo87 Feb 5, 2017

bardzusny Feb 5, 2017

bardzusny Feb 5, 2017

Turbo87 Feb 5, 2017

bardzusny left a comment •

edited

Loading

bardzusny left a comment

Report correct positions for "order-in-*" rules #15

Report correct positions for "order-in-*" rules #15

Conversation

Turbo87 commented Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalsnik left a comment

Choose a reason for hiding this comment

bardzusny Feb 5, 2017 • edited Loading

Choose a reason for hiding this comment

bardzusny Feb 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bardzusny left a comment • edited Loading

Choose a reason for hiding this comment

bardzusny left a comment

Choose a reason for hiding this comment

Turbo87 commented Feb 3, 2017 •

edited

Loading

bardzusny Feb 5, 2017 •

edited

Loading

bardzusny Feb 5, 2017 •

edited

Loading

bardzusny left a comment •

edited

Loading