JSHint Option Discussion #1219

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
10 participants
@scottgonzalez
Owner

scottgonzalez commented Apr 4, 2013

Ignore the commit. I just wanted to have this discussion in GitHub and we don't have issues turned on :-)

The following options are currently used in our various code repos:

option jquery jquery-ui jquery-mobile sizzle qunit
bitwise
boss
camelcase
curly
eqnull
eqeqeq
expr
forin
immed
latedef
newcap
noarg
noempty
nonew
onevar
plusplus
proto
quotmark
smarttabs
sub
trailing
undef
unused

The three columns for each project are: root, js source, tests.

  • jquery-mobile doesn't have a .jshintrc specifically for tests.
  • sizzle is root, speed, tests.
  • qunit doesn't have a .jshintrc specifically for js source.

✘ means the option is explicitly disabled (set to false).


I'd like us to agree on a baseline of options that must be enabled in all projects. Once we have that list, we should update our .jshintrc files to have the options alphabetized and grouped:

{
    "common1": true,
    "common2": true,
    "common3": true,

    "repoSpecific1": true,
    "repoSpecific2": true,

    "globals": {
        "repoGlobal1": true,
        "repoGlobal2": false
    }
}

Proposed common options:

{
    "boss": true,
    "curly": true,
    "eqeqeq": true,
    "eqnull": true,
    "expr": true,
    "immed": true,
    "noarg": true,
    "onevar": true,
    "quotmark": "double",
    "smarttabs": true,
    "trailing": true,
    "undef": true,
    "unused": true
}
@rwaldron

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 4, 2013

Member

Just to be clear, you mean this as a baseline only, ie. the minimum requirement?

Member

rwaldron commented Apr 4, 2013

Just to be clear, you mean this as a baseline only, ie. the minimum requirement?

@dmethvin

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Apr 4, 2013

Owner

The idea is that we come up with some project-wide defaults that meet our style needs, and then each project can ad d to them as required. It would seem unusual to remove one of the project settings.

There's definitely some common ground here and the asymmetrical patterns are probably just current don't-cares more than a need to fix a specific problem. For example I suspect eqnull and eqeqeq can be used by everyone and we're almost there already.

Some of the settings may end up being specific to node vs browser as well.

Owner

dmethvin commented Apr 4, 2013

The idea is that we come up with some project-wide defaults that meet our style needs, and then each project can ad d to them as required. It would seem unusual to remove one of the project settings.

There's definitely some common ground here and the asymmetrical patterns are probably just current don't-cares more than a need to fix a specific problem. For example I suspect eqnull and eqeqeq can be used by everyone and we're almost there already.

Some of the settings may end up being specific to node vs browser as well.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 4, 2013

Owner

@rwldrn Yes, as @dmethvin says, the proposal at the bottom is a baseline that all projects must use. Then they can add to it in the repo-specific section. The reason for proposing a format for .jshintrc is so that it's easy to verify that all projects are using the baseline settings.

Owner

scottgonzalez commented Apr 4, 2013

@rwldrn Yes, as @dmethvin says, the proposal at the bottom is a baseline that all projects must use. Then they can add to it in the repo-specific section. The reason for proposing a format for .jshintrc is so that it's easy to verify that all projects are using the baseline settings.

@rwaldron

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Apr 4, 2013

Member

@scottgonzalez great, in that case +1

Member

rwaldron commented Apr 4, 2013

@scottgonzalez great, in that case +1

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Apr 4, 2013

Owner

I don't think we need immed in the baseline.

Owner

timmywil commented Apr 4, 2013

I don't think we need immed in the baseline.

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Apr 4, 2013

Owner

The rest sound good though.

Owner

timmywil commented Apr 4, 2013

The rest sound good though.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 4, 2013

Owner

I included immed because our style seems to require it. Whatever we choose, we should be consistent. So either we want the wrapping and should enable it or we don't want the wrapping and we should change all of our code.

Owner

scottgonzalez commented Apr 4, 2013

I included immed because our style seems to require it. Whatever we choose, we should be consistent. So either we want the wrapping and should enable it or we don't want the wrapping and we should change all of our code.

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Apr 4, 2013

Owner

Ah, I thought it enforced (function() {}()) rather than (function() {})() as well, but I guess not.

Owner

timmywil commented Apr 4, 2013

Ah, I thought it enforced (function() {}()) rather than (function() {})() as well, but I guess not.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 4, 2013

Owner

Oh, to be honest, I'm not sure. If it requires a specific form and it's not the form we use, then I agree we should leave it out.

Owner

scottgonzalez commented Apr 4, 2013

Oh, to be honest, I'm not sure. If it requires a specific form and it's not the form we use, then I agree we should leave it out.

@JamesMGreene

This comment has been minimized.

Show comment Hide comment
@JamesMGreene

JamesMGreene Apr 4, 2013

Member

JSHint's immed already supports both variations. See the "dogballs" issue: jshint/jshint#470

We should definitely have it turned on.

Also, as a side note, QUnit actually has a 3rd .jshintrc file in our addons/plugins dir.

Member

JamesMGreene commented Apr 4, 2013

JSHint's immed already supports both variations. See the "dogballs" issue: jshint/jshint#470

We should definitely have it turned on.

Also, as a side note, QUnit actually has a 3rd .jshintrc file in our addons/plugins dir.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 5, 2013

Owner

JSHint's immed already supports both variations. See the "dogballs" issue: jshint/jshint#470

great

We should definitely have it turned on.

agreed

Also, as a side note, QUnit actually has a 3rd .jshintrc file in our addons/plugins dir.

I intentionally skipped that one since I assume it's going away.

Owner

scottgonzalez commented Apr 5, 2013

JSHint's immed already supports both variations. See the "dogballs" issue: jshint/jshint#470

great

We should definitely have it turned on.

agreed

Also, as a side note, QUnit actually has a 3rd .jshintrc file in our addons/plugins dir.

I intentionally skipped that one since I assume it's going away.

@JamesMGreene

This comment has been minimized.

Show comment Hide comment
@JamesMGreene

JamesMGreene Apr 5, 2013

Member

I intentionally skipped that one since I assume it's going away.

Yes, just thought you may want to consider its settings, too.

Member

JamesMGreene commented Apr 5, 2013

I intentionally skipped that one since I assume it's going away.

Yes, just thought you may want to consider its settings, too.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Apr 5, 2013

Member

Using eqeqeq globally could require temporarily disabling it for oldIE specific cases. My pull request wrapping comparisons to window-like object into /* jshint eqeqeq: false */ was already merged to 1.x-master branch in jQuery. I don't have anything against it but it's sth that we have to be wary about.

All in all, proposed default options seem reasonable to me.

Member

mgol commented Apr 5, 2013

Using eqeqeq globally could require temporarily disabling it for oldIE specific cases. My pull request wrapping comparisons to window-like object into /* jshint eqeqeq: false */ was already merged to 1.x-master branch in jQuery. I don't have anything against it but it's sth that we have to be wary about.

All in all, proposed default options seem reasonable to me.

@jzaefferer

This comment has been minimized.

Show comment Hide comment
@jzaefferer

jzaefferer Apr 5, 2013

Owner

No objections against onevar from jquery or sizzle? Enabling that usually means rewriting a lot of lines. Which is usually worth it, since otherwise variable declarations are pretty random.

Owner

jzaefferer commented Apr 5, 2013

No objections against onevar from jquery or sizzle? Enabling that usually means rewriting a lot of lines. Which is usually worth it, since otherwise variable declarations are pretty random.

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Apr 5, 2013

Owner

Ah. right. Sizzle. Yea, onevar in Sizzle is not an option.

Owner

timmywil commented Apr 5, 2013

Ah. right. Sizzle. Yea, onevar in Sizzle is not an option.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 5, 2013

Owner

Yea, onevar in Sizzle is not an option.

Can you expand on this? Similarly, I'd like to know why Sizzle requires sub.

Owner

scottgonzalez commented Apr 5, 2013

Yea, onevar in Sizzle is not an option.

Can you expand on this? Similarly, I'd like to know why Sizzle requires sub.

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Apr 5, 2013

Member

sub is out right now because of @ChadKillingsworth's closure compiler contributions. We could probably work with onevar, though, unless jshint is terribly broken in implementing it.

Member

gibson042 commented Apr 5, 2013

sub is out right now because of @ChadKillingsworth's closure compiler contributions. We could probably work with onevar, though, unless jshint is terribly broken in implementing it.

@ChadKillingsworth

This comment has been minimized.

Show comment Hide comment
@ChadKillingsworth

ChadKillingsworth Apr 5, 2013

Contributor

It would be helpful if sub were not required as quoted property names are treated differently than unquoted ones in Closure-compiler. I realize this is a style issue, but both uglify and closure-compiler should make sure that this difference has no impact on the final code size. I'm doing my best right now to reduce the number of quoted properties though - but some are just required.

Contributor

ChadKillingsworth commented Apr 5, 2013

It would be helpful if sub were not required as quoted property names are treated differently than unquoted ones in Closure-compiler. I realize this is a style issue, but both uglify and closure-compiler should make sure that this difference has no impact on the final code size. I'm doing my best right now to reduce the number of quoted properties though - but some are just required.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 5, 2013

Owner

Is defining exports at the bottom an acceptable compromise?

(function() {
    /* jshint sub: true */
    Expr.find[ "ID" ] = Expr.find.ID;
})();
Owner

scottgonzalez commented Apr 5, 2013

Is defining exports at the bottom an acceptable compromise?

(function() {
    /* jshint sub: true */
    Expr.find[ "ID" ] = Expr.find.ID;
})();
@ChadKillingsworth

This comment has been minimized.

Show comment Hide comment
@ChadKillingsworth

ChadKillingsworth Apr 5, 2013

Contributor

Quite possibly, but I'll have to try it and see what happens. There's a few places in the jQuery source that I had to use quoted properties: css, event and manipulation off the top of my head. However these are only in my branch so I'm not sure whether that will impact the discussion.

I also might be able to handle this with the Closure-compiler @expose annotation - I just haven't tested that yet.

Contributor

ChadKillingsworth commented Apr 5, 2013

Quite possibly, but I'll have to try it and see what happens. There's a few places in the jQuery source that I had to use quoted properties: css, event and manipulation off the top of my head. However these are only in my branch so I'm not sure whether that will impact the discussion.

I also might be able to handle this with the Closure-compiler @expose annotation - I just haven't tested that yet.

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Apr 5, 2013

Owner

Is defining exports at the bottom an acceptable compromise?

Keep in mind Sizzle is not the only project that we'd like to have work in advanced mode, so I'd rather keep it simple and just use quotes. I certainly wouldn't want to include exports like that in the main Sizzle source. And even if it could just be in a separate file for build time, it would mean more maintenance across multiple projects.

As for onevar, we've tried to conform to it before and it's made the source less readable. Sizzle defines references to static methods in the same place as their definition, which is simpler to read. We could make it work, but there's little reason to do such an overhaul of the codebase.

Owner

timmywil commented Apr 5, 2013

Is defining exports at the bottom an acceptable compromise?

Keep in mind Sizzle is not the only project that we'd like to have work in advanced mode, so I'd rather keep it simple and just use quotes. I certainly wouldn't want to include exports like that in the main Sizzle source. And even if it could just be in a separate file for build time, it would mean more maintenance across multiple projects.

As for onevar, we've tried to conform to it before and it's made the source less readable. Sizzle defines references to static methods in the same place as their definition, which is simpler to read. We could make it work, but there's little reason to do such an overhaul of the codebase.

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Apr 5, 2013

Owner

Heh, you know what. I got that backwards. We've tried to conform to it before, and apparently we stuck with it. Whatevs.

Owner

timmywil commented Apr 5, 2013

Heh, you know what. I got that backwards. We've tried to conform to it before, and apparently we stuck with it. Whatevs.

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Apr 5, 2013

Owner

I had just woken up, don't judge me.

Owner

timmywil commented Apr 5, 2013

I had just woken up, don't judge me.

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Apr 5, 2013

Owner

Yea, turning onevar on in Sizzle passes without changes.

Owner

timmywil commented Apr 5, 2013

Yea, turning onevar on in Sizzle passes without changes.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 5, 2013

Owner

Ok, so far we have full agreement on the proposed baseline. Since sub isn't in that list, Sizzle is free to use it.

I'll leave this open for a few more days to give others time to respond.

Owner

scottgonzalez commented Apr 5, 2013

Ok, so far we have full agreement on the proposed baseline. Since sub isn't in that list, Sizzle is free to use it.

I'll leave this open for a few more days to give others time to respond.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Apr 5, 2013

Member

As for mangling, it can cause errors in UglifyJS in some specific use cases, see my bug report (I should have reported it in UglifyJS2, not there, my bad): https://github.com/mishoo/UglifyJS/issues/485. This resulted in the --screw-ie8 option (for a moment it was just --screw-ie but it was renamed on my request) that disables naming changes required by oldIE broken named function expression handling.

It could be used in jQuery 2.0, probably, just in case (it's safer if oldIE support is not required), once it gets support in grunt-contrib-uglify: gruntjs/grunt-contrib-uglify#40.

I don't know how closure compiler deals with that.

Member

mgol commented Apr 5, 2013

As for mangling, it can cause errors in UglifyJS in some specific use cases, see my bug report (I should have reported it in UglifyJS2, not there, my bad): https://github.com/mishoo/UglifyJS/issues/485. This resulted in the --screw-ie8 option (for a moment it was just --screw-ie but it was renamed on my request) that disables naming changes required by oldIE broken named function expression handling.

It could be used in jQuery 2.0, probably, just in case (it's safer if oldIE support is not required), once it gets support in grunt-contrib-uglify: gruntjs/grunt-contrib-uglify#40.

I don't know how closure compiler deals with that.

@Krinkle

This comment has been minimized.

Show comment Hide comment
@Krinkle

Krinkle Apr 5, 2013

Member

Once we settle on common settings, where will they be stored? I'd propose creating a page on http://contribute.jquery.org/ with an easy to copy/paste snippet?

Also, I've created a doodle for this: http://www.doodle.com/it7i4tqnpf86v7f9.

Though this is not a vote-based decision, it might make it easier to get a rough view of where we stand.

Member

Krinkle commented Apr 5, 2013

Once we settle on common settings, where will they be stored? I'd propose creating a page on http://contribute.jquery.org/ with an easy to copy/paste snippet?

Also, I've created a doodle for this: http://www.doodle.com/it7i4tqnpf86v7f9.

Though this is not a vote-based decision, it might make it easier to get a rough view of where we stand.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 6, 2013

Owner

Once we settle on common settings, where will they be stored? I'd propose creating a page on http://contribute.jquery.org/ with an easy to copy/paste snippet?

We can put it in our JS style guide. There's already a section for linting.

Also, I've created a doodle for this: http://www.doodle.com/it7i4tqnpf86v7f9.

That doesn't seem necessary. The doodle doesn't provide any way to discuss the options, which is what we're trying to do. That doodle also complicates the discussion since it includes all options, not the proposed baseline.

Owner

scottgonzalez commented Apr 6, 2013

Once we settle on common settings, where will they be stored? I'd propose creating a page on http://contribute.jquery.org/ with an easy to copy/paste snippet?

We can put it in our JS style guide. There's already a section for linting.

Also, I've created a doodle for this: http://www.doodle.com/it7i4tqnpf86v7f9.

That doesn't seem necessary. The doodle doesn't provide any way to discuss the options, which is what we're trying to do. That doodle also complicates the discussion since it includes all options, not the proposed baseline.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 9, 2013

Owner

Looks like we're in agreement, so I'm going to close this.

I updated jQuery UI yesterday: jquery/jquery-ui@8f93106

Owner

scottgonzalez commented Apr 9, 2013

Looks like we're in agreement, so I'm going to close this.

I updated jQuery UI yesterday: jquery/jquery-ui@8f93106

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Apr 9, 2013

Owner

Documented in our JS style guide.

Owner

scottgonzalez commented Apr 9, 2013

Documented in our JS style guide.

@JamesMGreene JamesMGreene referenced this pull request in qunitjs/qunit Apr 9, 2013

Closed

Update JSHint options to jQuery baseline #439

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment