Whitespace

[brettz9]

Build on the JSLint to also add whitespace checking and changes.

[jdorn]

I don't like how opinionated JSLint can get. I'd prefer something like JSHint instead which just catches common coding errors. It would also be good if this was built into the Grunt build process. I think the ideal build process would be:

1. Run JSHint on source files. If there are errors, stop.
2. Concat files
3. Minify with UglifyJS

I just ran a quick test and JSHint found 47 errors total in the source files. Once those are fixed, this build process should stop new errors from being introduced in the future.

[brettz9]

Sure... Well, I think JSHint just provides more tolerant customization options. I believe valid JSLint should be valid JSHint, and my first PR did not include all of the more finicky whitespace requirements. Are there specific coding practices you did not want enforced in my first PR? If so, I can see if JSHint allows disabling them.

[jdorn]

I'm fine with all of the default options used in https://github.com/gruntjs/grunt-contrib-jshint to start at least.

From the first PR, I don't really see the need for "use strict" and wrapping each individual source file in a closure. Some things also seem like purely cosmetic changes, like changing "editor_class" to "EditorClass" and requiring curly braces around single-line if statements.

I think the real benefit of this should be catching things like duplicate var declarations, missing semicolons, variables used out of scope, etc..

[brettz9]

Ok, thanks. Will take a look. Did you notice my question in that PR about:

  validate: function(value) {
    if(!this.ready) throw "JSON Editor not ready yet.  Listen for 'ready' event before validating";

    // Custom value
    if(arguments.length === 1) {
      return this.validator.validate(arguments[1]);
    }
    // Current value (use cached result)
    else {
      return this.validation_results;
    }
  }

If arguments.length is only one, it should be arguments[0], no? Otherwise, you are just passing undefined. (And in that case, value should be usable instead of arguments[0].)

[jdorn]

Yeah, that looks like a typo. It should be value instead of arguments[1].

[brettz9]

It can of course be up for debate, but his rationale about curly braces is that it is too easy for people who try to add a line of code to a statement without braces, intending to add it into the block (e.g., as another statement inside an if) to instead close the old block and create a new one.

Use of CamelCase for constructors also could arguably aid development because others familiar with this convention can identify the uses of a function more quickly.

But I understand everyone has their preferences, and it's of course your project! :) I'll look to make a new PR on these points.

[brettz9]

Also, if Grunt is using the default options of JSHint itself, then at least for the JSHint version in Notepad++ or on jshint.com, that error with arguments[1] would not have been caught by the default options.

If you are open to it, I can see about adding JSHint directives which act more like JSLint minus the items you already indicated you disfavored, and then you can yourself look at the new results to see whether there are still items you wish to be ignored.

[jdorn]

I added jshint to the Grunt build process. It checks each file before concatenation and then the dist file at the end. If it encounters any linting errors, it will exit.

It's using the default JSHint options right now. I'd be open to changing the options though if there is a compelling reason to do so.

[brettz9]

Ok, that's cool by me, thanks!

I do have three requests as far as JSHint:

The version I have using a Notepad++ plugin (which isn't letting me update to the latest version of JSHint) complains now only about an issue I partially fixed in #130 (I would need to lint a few other files besides the validator) which is that in at least older ECMAScript, reserved keywords could not be used even as object properties (older IE actually will not allow execution and a lot of syntax highlighters display them differently), so since enum was a reserved keyword in JS, schema.enum would need to be escaped as schema['enum'] (otherwise, for non-keywords, the linting tools prefer the shorter form). Are you ok with my doing a PR to make this particular change everywhere it occurs?

Secondly, I see in the presumably latest JSHint at JSHint.com, when I paste the jsoneditor.js code there, there are two kinds of complaints:

1. About arguments.callee in Class. This is not allowed in 'strict' mode, FYI, as it is supposed to be difficult for JS engines to work around for optimizations. But I could add the http://jshint.com/docs/options/#noarg directive , perhaps only around this code, i.e.,:

    /*jshint noarg:false*/
    Class.extend = arguments.callee;
    /*jshint noarg:true*/

1. There are a ton of complaints about filtering for-in (e.g., with hasOwnProperty). In at least a good number of cases, your code is already doing this, but it doesn't apparently recognize the approach of using if(!value.hasOwnProperty(i)) continue; and instead expects if(value.hasOwnProperty(i)) { /*The code which is executed */};. I know the former is more elegant as it avoids indentation, but for some reason JSLint (on which JSHint was of course based) doesn't like continue statements so apparently didn't allow for this exception. I don't know if you want to configure JSHint to avoid these for-in errors (and possibly miss legitimate warnings) or have the code adjusted to avoid using continue in these cases. (or as in Support validation keywords for arrays #1, temporarily add an argument to ignore the error)

[jdorn]

I don't want to support any build tools outside of Grunt (i.e. Notepad++). Grunt currently comes back with no linting errors. If there's a compelling reason to make JSHint more strict, I'll consider adding options in Gruntfile.js. I don't want to use inline directives if at all possible.

JSON Editor doesn't support old IE, so schema.enum should be fine. I don't see a good reason to change this.

I'm aware of the arguments.callee problem and would like to find a workaround eventually. Grunt doesn't complain about it, so the jshint directives are not needed.

For the for-in loops, again Grunt doesn't complain, so I'd like to leave them as is. No need to add jshint directives or anything.

[brettz9]

Ok

[brettz9]

So I'll redo #130 then without the reserved word changes.