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

Add new 'no-unnecessary-route-path-option' rule #370

Merged
merged 1 commit into from Jan 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Expand Up @@ -96,6 +96,7 @@ The `--fix` option on the command line automatically fixes problems reported by
| :white_check_mark::wrench: | [no-old-shims](./docs/rules/no-old-shims.md) | Prevents usage of old shims for modules |
| :white_check_mark: | [no-on-calls-in-components](./docs/rules/no-on-calls-in-components.md) | Prevents usage of `on` to call lifecycle hooks in components |
| :white_check_mark: | [no-restricted-resolver-tests](./docs/rules/no-restricted-resolver-tests.md) | Prevents the use of patterns that use the restricted resolver in tests. |
| | [no-unnecessary-route-path-option](./docs/rules/no-unnecessary-route-path-option.md) | Disallow unnecessary route `path` option |
| :wrench: | [use-ember-get-and-set](./docs/rules/use-ember-get-and-set.md) | Enforces usage of Ember.get and Ember.set |


Expand Down
32 changes: 32 additions & 0 deletions docs/rules/no-unnecessary-route-path-option.md
@@ -0,0 +1,32 @@
## Disallow unnecessary route `path` option

### Rule name: `no-unnecessary-route-path-option`

When defining a route, it's not necessary to specify the `path` option if it matches the route name.

### Rule Details

Examples of **incorrect** code for this rule:

```js
this.route('blog-posts', { path: '/blog-posts' });
```

Examples of **correct** code for this rule:

```js
this.route('blog-posts');
```

```js
this.route('blog-posts', { path: '/blog' });
```

### References

* [Ember Routing Guide](https://guides.emberjs.com/release/routing/)

### Related Rules

* [no-capital-letters-in-routes](no-capital-letters-in-routes.md)
* [routes-segments-snake-case](routes-segments-snake-case.md)
1 change: 1 addition & 0 deletions lib/index.js
Expand Up @@ -32,6 +32,7 @@ module.exports = {
'no-side-effects': require('./rules/no-side-effects'),
'no-test-and-then': require('./rules/no-test-and-then'),
'no-test-import-export': require('./rules/no-test-import-export'),
'no-unnecessary-route-path-option': require('./rules/no-unnecessary-route-path-option'),
'order-in-components': require('./rules/order-in-components'),
'order-in-controllers': require('./rules/order-in-controllers'),
'order-in-models': require('./rules/order-in-models'),
Expand Down
1 change: 1 addition & 0 deletions lib/recommended-rules.js
Expand Up @@ -34,6 +34,7 @@ module.exports = {
"ember/no-side-effects": "error",
"ember/no-test-and-then": "off",
"ember/no-test-import-export": "off",
"ember/no-unnecessary-route-path-option": "off",
"ember/order-in-components": "off",
"ember/order-in-controllers": "off",
"ember/order-in-models": "off",
Expand Down
61 changes: 61 additions & 0 deletions lib/rules/no-unnecessary-route-path-option.js
@@ -0,0 +1,61 @@
'use strict';

const utils = require('../utils/utils');
const ember = require('../utils/ember');

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

const ERROR_MESSAGE = 'Do not provide unnecessary `path` option which matches the route name.';

module.exports = {
meta: {
docs: {
description: 'Disallow unnecessary route `path` option',
category: 'Best Practices',
recommended: false
},
fixable: null,
ERROR_MESSAGE
},

create(context) {
return {
CallExpression(node) {
if (!ember.isRoute(node)) {
return;
}

const hasExplicitPathOption = utils.isObjectExpression(node.arguments[1]) && hasPropertyWithKeyName(node.arguments[1], 'path');
if (!hasExplicitPathOption) {
return;
}

const pathOptionNode = getPropertyByKeyName(node.arguments[1], 'path');
const pathOptionValue = pathOptionNode.value.value;
const routeName = node.arguments[0].value;

if (pathMatchesRouteName(pathOptionValue, routeName)) {
context.report({
node: pathOptionNode,
message: ERROR_MESSAGE
});
}
}
};
}
};

function hasPropertyWithKeyName(objectExpression, keyName) {
return getPropertyByKeyName(objectExpression, keyName) !== undefined;
}

function getPropertyByKeyName(objectExpression, keyName) {
return objectExpression.properties.find(property => property.key.name === keyName);
}

function pathMatchesRouteName(path, routeName) {
const pathWithoutOptionalLeadingSlash = path.substring(0, 1) === '/' ? path.substring(1) : path;
return pathWithoutOptionalLeadingSlash === routeName;
}
67 changes: 67 additions & 0 deletions tests/lib/rules/no-unnecessary-route-path-option.js
@@ -0,0 +1,67 @@
//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const rule = require('../../../lib/rules/no-unnecessary-route-path-option');
const RuleTester = require('eslint').RuleTester;

const { ERROR_MESSAGE } = rule;

//------------------------------------------------------------------------------
// Tests
//------------------------------------------------------------------------------

const ruleTester = new RuleTester();
ruleTester.run('no-unnecessary-route-path-option', rule, {
valid: [
'this.route("blog");',
'this.route("blog", function() {});',
'this.route("blog", { path: "/" });',
'this.route("blog", { path: "blog-posts" });',
'this.route("blog", { path: "blog-posts" }, function() {});',
'this.route("blog", { path: "/blog-posts" });',
'this.route("blog", { path: "blog-posts", otherOption: true });',

// With dynamic segment:
'this.route("blog", { path: ":blog" });',
'this.route("blog", { path: "/:blog" });',
'this.route("blog", { path: "blog/:blog_id" });',

// With wildcard segment:
'this.route("blog", { path: "*blog" });',
'this.route("blog", { path: "/*blog" });',
'this.route("blog", { path: "blog/*blog" });',

// Not Ember's route function:
'test();',
"test('blog');",
"test('blog', { path: 'blog' });",
"test('blog', { path: '/blog' });",
"this.test('blog');",
"this.test('blog', { path: 'blog' });",
"this.test('blog', { path: '/blog' });",
"MyClass.route('blog');",
"MyClass.route('blog', { path: 'blog' });",
"MyClass.route('blog', { path: '/blog' });",
"route.unrelatedFunction('blog', { path: 'blog' });",
"this.route.unrelatedFunction('blog', { path: 'blog' });"
],
invalid: [
{
code: 'this.route("blog", { path: "blog" });',
errors: [{ message: ERROR_MESSAGE, type: 'Property' }]
},
{
code: 'this.route("blog", { path: "blog" }, function() {});',
errors: [{ message: ERROR_MESSAGE, type: 'Property' }]
},
{
code: 'this.route("blog", { path: "/blog" });',
errors: [{ message: ERROR_MESSAGE, type: 'Property' }]
},
{
code: 'this.route("blog", { path: "/blog", otherOption: true });',
errors: [{ message: ERROR_MESSAGE, type: 'Property' }]
}
]
});