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

Unused var check in argument list only catches last param #778

Closed
mbulman opened this Issue Dec 17, 2012 · 10 comments

Comments

Projects
None yet
4 participants
@mbulman
Contributor

mbulman commented Dec 17, 2012

Example that does throw an error:

function foo(a, b) {
  window.alert(a);
}
foo();

Example that should, but does not throw an error:

function foo(a, b) {
  window.alert(b);
}
foo();

Verified against jshint.com as of this writing.

@valueof

This comment has been minimized.

Show comment
Hide comment
@valueof

valueof Dec 17, 2012

Member

That's a feature, not a bug. :-) When I first implemented unused a lot of people complained because there is a common pattern in JS/Node community that goes like this:

run(function (err, result) {
  display(result);
});

So sometimes people don't care about err so I changed JSHint to assume that whenever an unused variable is followed by a used one it's intentional.

Member

valueof commented Dec 17, 2012

That's a feature, not a bug. :-) When I first implemented unused a lot of people complained because there is a common pattern in JS/Node community that goes like this:

run(function (err, result) {
  display(result);
});

So sometimes people don't care about err so I changed JSHint to assume that whenever an unused variable is followed by a used one it's intentional.

@valueof valueof closed this Dec 17, 2012

@mbulman

This comment has been minimized.

Show comment
Hide comment
@mbulman

mbulman Dec 17, 2012

Contributor

Seems like something better suited for an option, than changing global behavior for a single use case :)

eg, unused-args

Contributor

mbulman commented Dec 17, 2012

Seems like something better suited for an option, than changing global behavior for a single use case :)

eg, unused-args

@tupton

This comment has been minimized.

Show comment
Hide comment
@tupton

tupton Dec 17, 2012

Can this behavior be optional? Sometimes people do care about unused variables like err.

tupton commented Dec 17, 2012

Can this behavior be optional? Sometimes people do care about unused variables like err.

@valueof

This comment has been minimized.

Show comment
Hide comment
@valueof

valueof Dec 17, 2012

Member

I don't like adding new options until absolutely necessary. My logic was: lots of people complained about the use case above so I changed it and waited for complaints from the other side. You guys are the first ones since then. :-)

Perhaps after I'm done with 1.0.0 release I'll look into making unused a multiple choice option so that it could accept such values as 'all', 'noparams', etc. instead of just true/false.

Member

valueof commented Dec 17, 2012

I don't like adding new options until absolutely necessary. My logic was: lots of people complained about the use case above so I changed it and waited for complaints from the other side. You guys are the first ones since then. :-)

Perhaps after I'm done with 1.0.0 release I'll look into making unused a multiple choice option so that it could accept such values as 'all', 'noparams', etc. instead of just true/false.

@mbulman

This comment has been minimized.

Show comment
Hide comment
@mbulman

mbulman Dec 17, 2012

Contributor

If I submit a pull request making it multiple choice, think you'll accept it?

For the node use case you mentioned, did those people seem okay with function params not getting checked at all? Or did they still want the last one checked? The latter seems overly specific to me.

Contributor

mbulman commented Dec 17, 2012

If I submit a pull request making it multiple choice, think you'll accept it?

For the node use case you mentioned, did those people seem okay with function params not getting checked at all? Or did they still want the last one checked? The latter seems overly specific to me.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Dec 17, 2012

Contributor

Fwiw, +1000 on multiple choice for unused

Contributor

ljharb commented Dec 17, 2012

Fwiw, +1000 on multiple choice for unused

@valueof

This comment has been minimized.

Show comment
Hide comment
@valueof

valueof Dec 17, 2012

Member

@mbulman I'd hold on a pull request unless you're okay for it to be on hold until after 1.0.0 release. Some people were okay with params not getting checked at all but I thought my solution was a nice compromise.

Member

valueof commented Dec 17, 2012

@mbulman I'd hold on a pull request unless you're okay for it to be on hold until after 1.0.0 release. Some people were okay with params not getting checked at all but I thought my solution was a nice compromise.

@mbulman

This comment has been minimized.

Show comment
Hide comment
@mbulman

mbulman Dec 17, 2012

Contributor

@antonkovalyov I'm okay with it not making it in until after 1.0.0, if I know you're open to the idea in general. I can run a patched version locally until then

Contributor

mbulman commented Dec 17, 2012

@antonkovalyov I'm okay with it not making it in until after 1.0.0, if I know you're open to the idea in general. I can run a patched version locally until then

@valueof

This comment has been minimized.

Show comment
Hide comment
@valueof

valueof Dec 17, 2012

Member

Technical details aside, I'm open to the idea in general.

Member

valueof commented Dec 17, 2012

Technical details aside, I'm open to the idea in general.

@mbulman

This comment has been minimized.

Show comment
Hide comment
@mbulman

mbulman Feb 19, 2013

Contributor

#867

I played around with some ideas that involved making 'unused' a pure string, and even an array of strings, but they all just made it more convoluted than I think it needs to be.

Contributor

mbulman commented Feb 19, 2013

#867

I played around with some ideas that involved making 'unused' a pure string, and even an array of strings, but they all just made it more convoluted than I think it needs to be.

mbulman added a commit to mbulman/jshint that referenced this issue Feb 20, 2013

Option to warn on all unused function params
This allows passing a value other than true/false to the 'unused' option to allow for checking all function params.  For
example:

    function foo(a, b) {
		return b;
	}

If 'unused' === true, no warnings will be thrown, since 'b' is used.  If 'unused' === 'all', then a warning will be
thrown, since 'a' is not used.

References:
* Makes jshintGH-607 the default, but optional, rather than mandatory
* Closes jshintGH-778

mbulman added a commit to mbulman/jshint that referenced this issue Feb 27, 2013

Expand on the configurability of "unused"
This allows users to control what level of check to do for unused variables.  The options are:
* false: No checking is done
* "vars": Variables and functions are checked, but not function params
* "all-params": "vars" + function params
* "last-param": "vars" + last function param only (this is what true will default to)

Example:

    function foo(a, b) {
		return b;
	}

If 'unused' === true, or "last-param", no warnings will be thrown, since 'b' is used.

If 'unused' === 'all-params', then a warning will be thrown, since 'a' is not used.

References:
* Makes jshintGH-607 the default, but optional, rather than mandatory
* Closes jshintGH-778

mbulman added a commit to mbulman/jshint that referenced this issue Feb 27, 2013

Expand on the configurability of "unused"
This allows users to control what level of check to do for unused variables.  The options are:
* false: No checking is done
* "vars": Variables and functions are checked, but not function params
* "all-params": "vars" + function params
* "last-param": "vars" + last function param only (this is what true will default to)

Example:

    function foo(a, b) {
		return b;
	}

If 'unused' === true, or "last-param", no warnings will be thrown, since 'b' is used.

If 'unused' === 'all-params', then a warning will be thrown, since 'a' is not used.

References:
* Makes jshintGH-607 the default, but optional, rather than mandatory
* Closes jshintGH-778

mbulman added a commit to mbulman/jshint that referenced this issue Mar 5, 2013

Expand on the configurability of "unused"
This allows users to control what level of check to do for unused variables.  The options are:
* false: No checking is done
* true: "vars" + last function param only
* "vars": Variables and functions are checked, but not function params
* "strict": "vars" + function params

Example:

    function foo(a, b) {
		return b;
	}

If 'unused' === true, no warnings will be thrown, since 'b' is used.

If 'unused' === 'strict', then a warning will be thrown, since 'a' is not used.

References:
* Makes jshintGH-607 the default, but optional, rather than mandatory
* Closes jshintGH-778

jugglinmike added a commit to jugglinmike/jshint that referenced this issue Oct 21, 2014

Expand on the configurability of "unused"
This allows users to control what level of check to do for unused variables.  The options are:
* false: No checking is done
* true: "vars" + last function param only
* "vars": Variables and functions are checked, but not function params
* "strict": "vars" + function params

Example:

    function foo(a, b) {
		return b;
	}

If 'unused' === true, no warnings will be thrown, since 'b' is used.

If 'unused' === 'strict', then a warning will be thrown, since 'a' is not used.

References:
* Makes jshintGH-607 the default, but optional, rather than mandatory
* Closes jshintGH-778
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment