Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Less ambiguous option values #166

Closed
intuited opened this Issue · 24 comments

10 participants

@intuited

The current use of true and false as option values seems to indicate deviation from the "norm", defined for most options as Douglas Crockford's historic opinion of how JS should be written. This really only makes sense if there is a defined norm; jshint seems to consider the eradication, or at least mutability, of such a concept its raison d'être.

In any case, the use of true and false is confusing, because for some options true dictates that some restriction is enforced, whereas for others it means that a restriction is not enforced.

Being able to explicitly specify, say, warn or allow as the option value would make it easier to remember how to configure jshint and to read configuration files and comment-embedded options.

E.G.

/*jshint evil: allow, curly: allow, forin: warn */

would equate to

/*jshint evil: true, curly: false, forin: false */
@valueof
Owner

I agree with that. I would even change/combine some options so that they accept three possible states: error, warn, skip. However, we need to make sure that all those changes are backwards compatible.

Marking the issue as accepted.

@millermedeiros

+1 to better option values,right now it is too confusing...

+1 also to different kinds of errors, it will allow IDEs to generate different error messages and we could even have a flag to only report errors, warnings or everything.

Issue #177 is also related.

@intuited

I guess the comment "identifier string" (i.e. /*jshint) might have to be different so that older (including now-current) versions of jshint wouldn't spit out warnings/errors on currently invalid values. Maybe /*jshint-options? IIRC the parsing code ignores everything that doesn't match one of the comment identifiers followed by a space.

@millermedeiros

one problem is that we have too many different kinds of options... I could group them into 4 different groups so far (environments, allow, disallow and requirements), adding error, warn and skip wouldn't be enough to solve the ambiguity problem.

we can create a hash table to convert true/false into error or skip (based on the option type), so it would be backwards compatible and no need to create a new "identifier string".

@patrio

JSLint made changes to the option values today

@jsuder

Yeah, as @I-ARE-RIO said, Crockford has just made a huge change in the options - all "enforcing" options became "relaxing" options... see this commit. What are your thoughts on this, @antonkovalyov? Should JSHint follow suit?

@valueof
Owner

We can't do the same thing because, unlike Crockford, I don't want to break all setups that set those options in their source codes. I.e. if you had /*jslint newcap:true */ and assumed that it enforces constructor capitalization after the upgrade it will actually relax that rule which is not what you'd expect.

Also I like the proposition of "warn", "ignore" and "error" states instead of boolean true/false. That way we can actually preserve backwards compatibility by checking if the option is boolean (legacy) or a string (new system).

What do you think?

@jsuder

Yeah, I also thought that suddenly changing the API so that the options mean something completely opposite isn't very nice for the users... I found this when I ran JSLint and JSHint against my code and was surprised that JSLint reported so many errors.

Warn/ignore sounds like a very good solution to me. It's definitely more work on your side, but much more convenient for the users.

@valueof
Owner

Ya, that change is on my priority list.

@millermedeiros

just to remember that "warn", "ignore", "error" won't work for environment variables (node, browser, devel, dojo, jquery, etc...) and we should keep boolean values for them.

And that one thing that may be weird is having something like: /*jshint eqeqeq:error */ - by first looking at it I would expect that if you use === it would be considered an error but in fact it considers == an error, so it still ambiguous.

other options that would also be weird if used together with the proposed states:

  • curly
  • eqeqeq
  • forin (seems that we can't use for in at all)
  • immed
  • newcap
  • noarg (better name would be "argcall")
  • noempty
  • nonew (it sounds llike I can't use "new")
  • nomen
  • onevar (to make mutiple vars an error would need new option)
  • passfail (this should be a boolean)
  • regexp (this name is bad anyways. better name would be "regexdot", "regexperiod" )
  • strict
  • trailing
  • white

I still think that the problem is on the properties names and not on the fact that it is using boolean values.

@valueof
Owner

The fact that they are booleans prevents users from controlling whether a message should be simply a warning or a fatal error. I agree that names should be revisited as well.

@jsuder

What is the status of a possible options redesign? Is this still planned, or is something of this kind currently in progress, or have you given up? Have you decided in what direction should this go?

@valueof
Owner

Yes, this is still planned. I just need more time on my hands.

@goatslacker

+1 for this.

@millermedeiros

What if we create a wiki page to list possible names to the each property like:

browser:

  • env_browser (env = environment)
  • isbrowser

forin:

  • protoforin
  • unsafeforin

It will be easier to pick the good ones later if we have a list with many options and if more people help coming up with the names...

Just need to enable wiki pages on the repository settings and create a description on the top of the file telling that it is only a proposal and the file is being edited by many people, no one should remove names, just add things to the list..

@PSPadJSHint

+1 for this.

@ixti

+1 And an updated list of options will be also really appreciated, as I can't find full list of options at all (jshint-node has an example of configuration with keys not listed on official homepage o_O)

@valueof
Owner

What options are not on jshint.com?

@ixti
"sub"   : true,   // Tolerate all forms of subscript notation besides dot notation e.g. `dict['key']` instead of `dict.key`.
"maxerr": 100,    // Maximum error before stopping.
"indent": 2       // Specify indentation spacing
@valueof
Owner

Docs for sub are in there (http://cl.ly/2U143H0H030M1R1Z342t), I missed maxerr and indent though—will fix. Thanks!

@ixti

Ooops. And I missed sub ;)) Thanks :))

@valueof
Owner

Closing in favor of jshint-next.

@valueof valueof closed this
@ForbesLindesay

For anyone else who sees this and looks for jshint-next

@mzgol

I don't see this issue resolved in new jsHint... Was the decision reverted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.