From 5c8d60d5614d2349f5bda7bd5046e7f13e9fe287 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Thu, 27 Dec 2018 11:20:52 -0600 Subject: [PATCH] feat: rename 'assert-arg-order' rule to 'no-invalid-debug-function-arguments' to also detect invalid usages of 'deprecate' and 'warn'. --- README.md | 2 +- docs/rules/assert-arg-order.md | 29 ----- .../no-invalid-debug-function-arguments.md | 42 +++++++ lib/index.js | 2 +- lib/recommended-rules.js | 2 +- ...=> no-invalid-debug-function-arguments.js} | 25 ++-- tests/lib/rules/assert-arg-order.js | 75 ----------- .../no-invalid-debug-function-arguments.js | 116 ++++++++++++++++++ 8 files changed, 175 insertions(+), 118 deletions(-) delete mode 100644 docs/rules/assert-arg-order.md create mode 100644 docs/rules/no-invalid-debug-function-arguments.md rename lib/rules/{assert-arg-order.js => no-invalid-debug-function-arguments.js} (50%) delete mode 100644 tests/lib/rules/assert-arg-order.js create mode 100644 tests/lib/rules/no-invalid-debug-function-arguments.js diff --git a/README.md b/README.md index 8696f268d9..e5884b6757 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,6 @@ The `--fix` option on the command line automatically fixes problems reported by | | Rule ID | Description | |:---|:--------|:------------| -| | [assert-arg-order](./docs/rules/assert-arg-order.md) | Catch usages of Ember's `assert()` function that have the arguments passed in the wrong order. | | :white_check_mark: | [jquery-ember-run](./docs/rules/jquery-ember-run.md) | Prevents usage of jQuery without Ember Run Loop | | :white_check_mark: | [no-attrs-in-components](./docs/rules/no-attrs-in-components.md) | Disallow usage of this.attrs in components | | :white_check_mark: | [no-attrs-snapshot](./docs/rules/no-attrs-snapshot.md) | Disallow use of attrs snapshot in `didReceiveAttrs` and `didUpdateAttrs` | @@ -112,6 +111,7 @@ The `--fix` option on the command line automatically fixes problems reported by | :white_check_mark: | [no-duplicate-dependent-keys](./docs/rules/no-duplicate-dependent-keys.md) | Disallow repeating dependent keys | | :wrench: | [no-ember-super-in-es-classes](./docs/rules/no-ember-super-in-es-classes.md) | Prevents use of `this._super` in ES class methods | | :white_check_mark: | [no-ember-testing-in-module-scope](./docs/rules/no-ember-testing-in-module-scope.md) | Prevents use of Ember.testing in module scope | +| | [no-invalid-debug-function-arguments](./docs/rules/no-invalid-debug-function-arguments.md) | Catch usages of Ember's `assert()` / `warn()` / `deprecate()` functions that have the arguments passed in the wrong order. | | :white_check_mark: | [no-side-effects](./docs/rules/no-side-effects.md) | Warns about unexpected side effects in computed properties | | :white_check_mark: | [require-super-in-init](./docs/rules/require-super-in-init.md) | Enforces super calls in init hooks | | :white_check_mark: | [routes-segments-snake-case](./docs/rules/routes-segments-snake-case.md) | Enforces usage of snake_cased dynamic segments in routes | diff --git a/docs/rules/assert-arg-order.md b/docs/rules/assert-arg-order.md deleted file mode 100644 index 93d03d2fc9..0000000000 --- a/docs/rules/assert-arg-order.md +++ /dev/null @@ -1,29 +0,0 @@ -# Catch usages of Ember's `assert()` function that have the arguments passed in the wrong order. (assert-arg-order) - -This rule aims to catch a common mistake when using Ember's `assert(String description, Boolean condition)` function. When writing an `assert`, the author may mistakenly pass the arguments in the reverse order, and not notice because the `assert` will pass with a truthy string as its second argument. - -## Rule Details - -Examples of **incorrect** code for this rule: - -```js -import { assert } from '@ember/debug'; - -... - -assert(label, 'Label must be present.'); -``` - -Examples of **correct** code for this rule: - -```js -import { assert } from '@ember/debug'; - -... - -assert('Label must be present.', label); -``` - -## Further Reading - -* See the [documentation](https://www.emberjs.com/api/ember/release/functions/@ember%2Fdebug/assert) for the Ember `assert` function. diff --git a/docs/rules/no-invalid-debug-function-arguments.md b/docs/rules/no-invalid-debug-function-arguments.md new file mode 100644 index 0000000000..02e1c395d8 --- /dev/null +++ b/docs/rules/no-invalid-debug-function-arguments.md @@ -0,0 +1,42 @@ +# Catch usages of Ember's `assert()` / `warn()` / `deprecate()` functions that have the arguments passed in the wrong order. (no-invalid-debug-function-arguments) + +This rule aims to catch a common mistake when using Ember's debug functions: +* `assert(String description, Boolean condition)` +* `warn(String description, Boolean condition, Object options)` +* `deprecate(String description, Boolean condition, Object options)` + +When calling one of these functions, the author may mistakenly pass the `description` and `condition` arguments in the reverse order, and not notice because the function will be silent with a truthy string as the `condition`. + +## Rule Details + +Examples of **incorrect** code for this rule: + +```js +import { assert, warn } from '@ember/debug'; +import { deprecate } from '@ember/application/deprecations'; + +... + +assert(label, 'Label must be present.'); +warn(label, 'Label must be present.', { id: 'missing-label' }); +deprecate(title, 'Title is no longer supported.', { id: 'unwanted-title', until: 'some-version' }); +``` + +Examples of **correct** code for this rule: + +```js +import { assert, warn } from '@ember/debug'; +import { deprecate } from '@ember/application/deprecations'; + +... + +assert('Label must be present.', label); +warn('Label must be present.', label, { id: 'missing-label' }); +deprecate('Title is no longer supported.', title, { id: 'unwanted-title', until: 'some-version' }); +``` + +## Further Reading + +* See the [documentation](https://www.emberjs.com/api/ember/release/functions/@ember%2Fdebug/assert) for the Ember `assert` function. +* See the [documentation](https://www.emberjs.com/api/ember/release/functions/@ember%2Fdebug/warn) for the Ember `warn` function. +* See the [documentation](https://emberjs.com/api/api/ember/release/functions/@ember%2Fapplication%2Fdeprecations/deprecate) for the Ember `deprecate` function. diff --git a/lib/index.js b/lib/index.js index 39962ed625..7fd09f2657 100644 --- a/lib/index.js +++ b/lib/index.js @@ -4,7 +4,6 @@ module.exports = { rules: { 'alias-model-in-controller': require('./rules/alias-model-in-controller'), - 'assert-arg-order': require('./rules/assert-arg-order'), 'avoid-leaking-state-in-components': require('./rules/avoid-leaking-state-in-components'), 'avoid-leaking-state-in-ember-objects': require('./rules/avoid-leaking-state-in-ember-objects'), 'avoid-using-needs-in-controllers': require('./rules/avoid-using-needs-in-controllers'), @@ -23,6 +22,7 @@ module.exports = { 'no-empty-attrs': require('./rules/no-empty-attrs'), 'no-function-prototype-extensions': require('./rules/no-function-prototype-extensions'), 'no-global-jquery': require('./rules/no-global-jquery'), + 'no-invalid-debug-function-arguments': require('./rules/no-invalid-debug-function-arguments'), 'no-jquery': require('./rules/no-jquery'), 'no-new-mixins': require('./rules/no-new-mixins'), 'no-observers': require('./rules/no-observers'), diff --git a/lib/recommended-rules.js b/lib/recommended-rules.js index bf184c58ca..ec69ec3fd4 100644 --- a/lib/recommended-rules.js +++ b/lib/recommended-rules.js @@ -6,7 +6,6 @@ */ module.exports = { "ember/alias-model-in-controller": "off", - "ember/assert-arg-order": "off", "ember/avoid-leaking-state-in-components": "off", "ember/avoid-leaking-state-in-ember-objects": "error", "ember/avoid-using-needs-in-controllers": "error", @@ -25,6 +24,7 @@ module.exports = { "ember/no-empty-attrs": "off", "ember/no-function-prototype-extensions": "error", "ember/no-global-jquery": "error", + "ember/no-invalid-debug-function-arguments": "off", "ember/no-jquery": "off", "ember/no-new-mixins": "off", "ember/no-observers": "error", diff --git a/lib/rules/assert-arg-order.js b/lib/rules/no-invalid-debug-function-arguments.js similarity index 50% rename from lib/rules/assert-arg-order.js rename to lib/rules/no-invalid-debug-function-arguments.js index 6ce5f0202a..e5f6065919 100644 --- a/lib/rules/assert-arg-order.js +++ b/lib/rules/no-invalid-debug-function-arguments.js @@ -7,23 +7,26 @@ const emberUtils = require('../utils/ember'); // Rule Definition //------------------------------------------------------------------------------ -const ERROR_MESSAGE = "Usage of Ember's assert(String description, Boolean condition) function has its arguments passed in the wrong order."; +const ERROR_MESSAGE = "Usage of Ember's `assert` / `warn` / `deprecate` function has its arguments passed in the wrong order. `String description` should come before `Boolean condition`."; +const DEBUG_FUNCTIONS = ['assert', 'deprecate', 'warn']; module.exports = { meta: { docs: { - description: "Catch usages of Ember's `assert()` function that have the arguments passed in the wrong order.", + description: "Catch usages of Ember's `assert()` / `warn()` / `deprecate()` functions that have the arguments passed in the wrong order.", category: 'Possible Errors', recommended: false }, - fixable: null, - ERROR_MESSAGE + fixable: null }, + ERROR_MESSAGE, + DEBUG_FUNCTIONS, + create(context) { return { CallExpression(node) { - if (isAssertFunctionWithReversedArgs(node)) { + if (isDebugFunctionWithReversedArgs(node)) { context.report({ node, message: ERROR_MESSAGE @@ -34,18 +37,18 @@ module.exports = { } }; -function isAssertFunctionWithReversedArgs(node) { +function isDebugFunctionWithReversedArgs(node) { return ( - isAssertFunction(node) && - node.arguments.length === 2 && + isDebugFunction(node) && + node.arguments.length >= 2 && !isString(node.arguments[0]) && isString(node.arguments[1]) ); } -function isAssertFunction(node) { - // Detect `Ember.assert()` or `assert()`. - return emberUtils.isModule(node, 'assert'); +function isDebugFunction(node) { + // Example: Detects `Ember.assert()` or `assert()`. + return DEBUG_FUNCTIONS.some(debugFunction => emberUtils.isModule(node, debugFunction)); } function isString(node) { diff --git a/tests/lib/rules/assert-arg-order.js b/tests/lib/rules/assert-arg-order.js deleted file mode 100644 index f46c89c4a7..0000000000 --- a/tests/lib/rules/assert-arg-order.js +++ /dev/null @@ -1,75 +0,0 @@ -//------------------------------------------------------------------------------ -// Requirements -//------------------------------------------------------------------------------ - -const rule = require('../../../lib/rules/assert-arg-order'); -const RuleTester = require('eslint').RuleTester; - -const { ERROR_MESSAGE } = rule; - -//------------------------------------------------------------------------------ -// Tests -//------------------------------------------------------------------------------ - -const ruleTester = new RuleTester({ - parserOptions: { - ecmaVersion: 2015, - sourceType: 'module' - } -}); -ruleTester.run('assert-arg-order', rule, { - valid: [ - { - code: "myFunction(true, 'My string.');" - }, - { - code: "Ember.myFunction(true, 'My string.');" - }, - { - code: "OtherClass.assert(true, 'My string.');" - }, - { - code: "assert('My description.');" - }, - { - code: "Ember.assert('My description.');" - }, - { - code: "assert('My description.', true);" - }, - { - code: "Ember.assert('My description.', true);" - }, - { - code: "const CONDITION = true; assert('My description.', CONDITION);" - }, - { - code: "const DESCRIPTION = 'description'; assert(DESCRIPTION, true);" - }, - { - code: "const DESCRIPTION = 'description'; const CONDITION = true; assert(DESCRIPTION, CONDITION);" - } - ], - invalid: [ - { - code: "assert(true, 'My description.');", - errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] - }, - { - code: "Ember.assert(true, 'My description.');", - errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] - }, - { - code: 'assert(true, `My ${123} description.`);', // eslint-disable-line no-template-curly-in-string - errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] - }, - { - code: "const CONDITION = true; assert(CONDITION, 'My description.');", - errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] - }, - { - code: 'const CONDITION = true; assert(CONDITION, `My ${123} description.`);', // eslint-disable-line no-template-curly-in-string - errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] - } - ] -}); diff --git a/tests/lib/rules/no-invalid-debug-function-arguments.js b/tests/lib/rules/no-invalid-debug-function-arguments.js new file mode 100644 index 0000000000..45b5f4a7b8 --- /dev/null +++ b/tests/lib/rules/no-invalid-debug-function-arguments.js @@ -0,0 +1,116 @@ +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/no-invalid-debug-function-arguments'); +const RuleTester = require('eslint').RuleTester; + +const { DEBUG_FUNCTIONS, ERROR_MESSAGE } = rule; + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const VALID_USAGES_BASIC = [ + { + code: "myFunction(true, 'My string.');" + }, + { + code: "Ember.myFunction(true, 'My string.');" + } +]; + +const VALID_USAGES_FOR_EACH_DEBUG_FUNCTION = flatten(DEBUG_FUNCTIONS.map(debugFunction => [ + { + code: `OtherClass.${debugFunction}(true, 'My string.');` + }, + { + code: `${debugFunction}();` + }, + { + code: `Ember.${debugFunction}();` + }, + { + code: `${debugFunction}('My description.');` + }, + { + code: `Ember.${debugFunction}('My description.');` + }, + { + code: `${debugFunction}('My description.', true);` + }, + { + code: `Ember.${debugFunction}('My description.', true);` + }, + { + code: `${debugFunction}('My description.', true, { id: 'some-id' });` + }, + { + code: `Ember.${debugFunction}('My description.', true, { id: 'some-id' });` + }, + { + code: `const CONDITION = true; ${debugFunction}('My description.', CONDITION);` + }, + { + code: `const DESCRIPTION = 'description'; ${debugFunction}(DESCRIPTION, true);` + }, + { + code: `const DESCRIPTION = 'description'; const CONDITION = true; ${debugFunction}(DESCRIPTION, CONDITION);` + } +])); + +const VALID_USAGES = [ + ...VALID_USAGES_BASIC, + ...VALID_USAGES_FOR_EACH_DEBUG_FUNCTION +]; + +const INVALID_USAGES = flatten(DEBUG_FUNCTIONS.map(debugFunction => [ + { + code: `${debugFunction}(true, 'My description.');`, + errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] + }, + { + code: `Ember.${debugFunction}(true, 'My description.');`, + errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] + }, + { + code: `${debugFunction}(true, 'My description.', { id: 'some-id' });`, + errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] + }, + { + code: `Ember.${debugFunction}(true, 'My description.', { id: 'some-id' });`, + errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] + }, + { + code: `${debugFunction}(true, \`My \${123} description.\`);`, + errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] + }, + { + code: `const CONDITION = true; ${debugFunction}(CONDITION, 'My description.');`, + errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] + }, + { + code: `const CONDITION = true; ${debugFunction}(CONDITION, \`My \${123} description.\`);`, + errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }] + } +])); + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 2015, + sourceType: 'module' + } +}); + +ruleTester.run('no-invalid-debug-function-arguments', rule, { + valid: VALID_USAGES, + invalid: INVALID_USAGES +}); + +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +function flatten(arr) { + return arr.reduce((acc, val) => acc.concat(val)); +}