New JSHint options

CesiumJS
1

Cesium devs,

The latest version of the Eclipse JSHint plugin (http://github.eclipsesource.com/jshint-eclipse/install.html) updates the bundled version of JSHint from r07 to r12, which brings some new options that we could consider enabling. Unfortunately, each of them have some caveats that mean I am not going to simply turn them on without discussion.

camelcase: Warns if variables are not either camelCase or UPPER_CASE. Sounds good, since this is already what we do, except for uniforms, which are all like czm_inverseModelView or u_nightTexture. I don’t think we want to rename all the uniforms, though.

quotmark: Warns when mixing double/single quotes. This also is what we already do, except it doesn’t know about the thing where we use single quotes everywhere, but put “use strict” in double-quotes, which honestly is just something I made up, and have already encountered minor objections to. If we abandon that rule in favor of all single quotes, all the time, then we can turn this on, although there’s currently a bug preventing non-boolean JSHint options in the Eclipse plugin, but we’d be able to enforce consistency in a file with the boolean option.

unused: Warns on unused variables. This is the one I’ve been looking forward to, in the hopes of being able to discard the buggy Eclipse JSDT validation completely. A few issues here. “Unused” variables include unused function parameters, which is great on one hand because it lets us find unused modules in our gigantic require lists (though unfortunately only if the unused modules are at the end), but it also means that event handler callbacks must use or delete their parameters, for example, here movement would have to be deleted, even though there’s kind of an “interface” to these callback functions that is worth leaving in place for clarity. Someone reported this on the JSHint issues, but seems it was closed because the JSHint guy didn’t actually understand the problem.

Another issue is that unused globals are warned as well, for example, in Specs we generally list all the Jasmine globals (example), even though they’re not all used in a given test. I guess we could just go back to what we used to do, and only list globals that the particular test needs. I don’t want to add the Jasmine globals to the overall project list, because they should not be available to production code, and the Eclipse plugin doesn’t let you set options on a per-folder basis or anything convenient like that.

Thoughts?

2

Given other priorities at the moment, we might want to make a github issue and address this later if any of this takes a non-trivial amount of time.

Comments:

camelcase: Warns if variables are not either camelCase or UPPER_CASE. Sounds good, since this is already what we do, except for uniforms, which are all like czm_inverseModelView or u_nightTexture. I don’t think we want to rename all the uniforms, though.

The czm_ prefix is not going away, u_ is debatable, but generally useful. Dropping the underscore is possible, but is inconsistent with built-in GLSL variables, but not GLSL functions. Are exceptions allowed, e.g., *.czm_, *.u_, etc.?

unused: Warns on unused variables. This is the one I’ve been looking forward to, in the hopes of being able to discard the buggy Eclipse JSDT validation completely. A few issues here. “Unused” variables include unused function parameters, which is great on one hand because it lets us find unused modules in our gigantic require lists (though unfortunately only if the unused modules are at the end), but it also means that event handler callbacks must use or delete their parameters, for example, here movement would have to be deleted, even though there’s kind of an “interface” to these callback functions that is worth leaving in place for clarity. Someone reported this on the JSHint issues, but seems it was closed because the JSHint guy didn’t actually understand the problem.

Bummer. It’s a shame this isn’t more fine-grained, decoupling local variables from function arguments. We commonly copy and past event handler callbacks so it is handy to have them with their full declaration, and it is natural as a bunch of C++ developers.

Perhaps it is easy to change JSHint, and contribute the code back.

Another issue is that unused globals are warned as well, for example, in Specs we generally list all the Jasmine globals (example), even though they’re not all used in a given test. I guess we could just go back to what we used to do, and only list globals that the particular test needs. I don’t want to add the Jasmine globals to the overall project list, because they should not be available to production code, and the Eclipse plugin doesn’t let you set options on a per-folder basis or anything convenient like that.

Another bummer. I think it is useful and reasonable to list all Jasmine globals like we do now.

Patrick

quotmark: Warns when mixing double/single quotes. This also is what we already do, except it doesn’t know about the thing where we use single quotes everywhere, but put “use strict” in double-quotes, which honestly is just something I made up, and have already encountered minor objections to. If we abandon that rule in favor of all single quotes, all the time, then we can turn this on, although there’s currently a bug preventing non-boolean JSHint options in the Eclipse plugin, but we’d be able to enforce consistency in a file with the boolean option.

I’m OK with ‘use strict’, instead of “use strict.” I don’t expect much - well, any - resistance here.

3

Thanks for pointing out this update. While I’m not contributing to Cesium code (yet), turning on all the fascist warnings has really helped pointing out questionable bits of my own code. Integrating it with Emacs has been great for keeping me honest. :slight_smile: