Add rules for prohibiting unused vars/expressions

[haroldtreen]

Add no-unused-* rules

Status: Ready for Review
Reviewers: @jansepar @donnielrt @marlowpayne

Changes:

• Add rules for no-unused-variables and no-unused-expressions

I have found these rules especially useful when they detect imports I don't use:
eg.

define(['somethingUsed', 'somethingUnused'], function(used, unused) { // Linting error!
      used();
});

// OR

var used = require('used');
var unused = require('unused'); // Linting error!

used();

[jansepar]

3am commits to the code style repo. Welcome fellow night time coder :)

:+1 thx Harold!

[marlowpayne]

Dang you guys be crazy with late night commits.

I'm wondering if these rules would break a common convention we sometimes use where we require in, but don't end up using the var for anything except side effects. For example selector-utils, or Deckard:

define([
    '$',
    'fastclick',
    'deckard'
],
function(
    $,
    fastclick
) {
    var globalUI = function() {
...

Also, these new rules should probably be run against a newly generated Adaptive project to check if anything needs updating in the generator to conform to the new ruleset.

[jansepar]

For the ones we bring in just for side effects, we shouldn't make an argument variable for it, and if we stick to that the linter should be fine. Like in you example above, we have no argument in the callback for deckard.

+1 to testing it in a newly generated project!

[haroldtreen]

Who can sleep when there may be unused variables in the wild!
Testing on a generated project sounds good! I shall do that.

As for side effect imports, Shawn's suggestion would work. I also wonder if the pattern of importing things to change the state of the world is good. Is there a way to make it more explicit that those packages are needed and do stuff?

Eg.

define([
    '$',
    'fastclick',
    'deckard'
], function($, fastclick, loadDeckard) { 
     loadDeckard();
});

Probably not a backwards compatible change, but I wonder if that would be a better system for future plugins.

[jansepar]

Totally - I think as much as possible libraries should be built as modules
that are side-effect less until invoked. We should change this for Deckard,
as we have done for many libraries of the past, such as capturing, the
Mobify Tag, etc. Unfortunately we don't own every library we use, so for
those that don't conform to a module system, we will have to just ensure we
don't put argument variables for them to conform to the style guide.
On Sat, Nov 14, 2015 at 11:53 AM Harold Treen [email protected]
wrote:

Who can sleep when there may be unused variables in the wild!
Testing on a generated project sounds good! I shall do that.

As for side effect imports, Shawn's suggestion would work. I also wonder
if the pattern of importing things to change the state of the world is
good. Is there a way to make it more explicit that those packages are
needed and do stuff?

Eg.

define([
'$',
'fastclick',
'deckard'

], function($, fastclick, loadDeckard) {
loadDeckard();
});

Probably not a backwards compatible change, but I wonder if that would be
a better system for future plugins.

—
Reply to this email directly or view it on GitHub
#78 (comment)
.

[jansepar]

What about when something you need is in the second argument? Say we wanted index but not num - a functional programming common practice is to use _ in it's place, but will the linter complain about that? If so I don't think we should add it to the linting rules.

[jeremywiebe]

@jansepar There are options to eslint we can use to ignore unused arguments to functions. That seems like it'd be a pragmatic compromise here.

"no-unused-vars": [2, {"vars": "all", "args": "none" }]

Reference: https://github.com/eslint/eslint/blob/master/docs/rules/no-unused-vars.md

Interestingly, I have no idea what the 2 is for in the array. All examples used 2 and the docs don't say what it's for. 😕

[marlowpayne]

@jeremywiebe The 2 indicates that a rule violation will surface an error (exit code of 1) instead of just a warning.

http://eslint.org/docs/user-guide/configuring#configuring-rules

[benlast]

👍 to using eslint to enforce this and related issues. We've found it very useful on Pusheen.

[marlowpayne]

I would suggest to allow the use of short circuits and ternary conditionals, since I see those a lot.

"no-unused-expressions": [2, { allowShortCircuit: true, allowTernary: true }]

[marlowpayne]

Even with options passed to both rules, a newly generated Adaptive.js project has JS lints. We should investigate what we need to change in our boilerplate code before we merge this.