Use jshint, clean up tests r=mikedeboer #106
Conversation
lambdabaa
commented
May 28, 2014
lambdabaa
commented
- Fixes Testing documentation and travis setup #103
- Fixes Automated linter #104 by configuring jshint and adding make task
- Fixes npm install dies trying to compile native bindings for time module #105 by specifying node <0.11 in npm engines
|
@mikedeboer Can you take a look and also turn on your travis endpoint? See https://travis-ci.org/profile/mikedeboer and toggle the little radio button for mikedeboer/jsDAV |
| @@ -72,7 +72,7 @@ exports.init = function(mongo, skipInit, callback) { | |||
| Async.list(operations) | |||
| .each(function(op, next) { | |||
| var coll = mongo.collection(op.collection); | |||
| if (op.type == "index") | |||
| if (op.type === "index") | |||
There was a problem hiding this comment.
See http://bonsaiden.github.io/JavaScript-Garden/#types.equality for a discussion
There was a problem hiding this comment.
I make a point of not knowing how == and != coerce things so that I can use my brain for other, more useful activities
|
I kind of like the style changes, except the I'd actually prefer to take a JSHint configuration that yields as few warnings as possible and work from there. |
| "bitwise" : true, // true: Prohibit bitwise operators (&, |, ^, etc.) | ||
| "camelcase" : false, // true: Identifiers must be in camelCase | ||
| "curly" : false, // true: Require {} for every new block or scope | ||
| "eqeqeq" : true, // true: Require triple equals (===) for comparison |
There was a problem hiding this comment.
Evert! 😄 I know it's best practice, but my short reasoning to allow it is as follows:
Know JS.
This might seem silly, but many of these 'rules' were invented to prevent Joe programmer to shoot themselves in the foot. We're not developing websites with JQuery here, we're coding a program in a set environment with parameters we control. Footguns will be caught during review.
The above might sound a bit gnarly, but I think 'best practices' need to be weighed and challenged for each use case.
I think JSHint is great and I'd really like to use it here, but it doesn't fix code. This PR doesn't fix any functional bug, just stylistic ones.
There was a problem hiding this comment.
It has very little to do with knowing or not knowing js. The JIT can heavily optimize for triple-equals.
When representing double, vs triple equals in machine code, the former is insanely complex and the latter is super simple. Using === is a very practical thing to do. Whenever I see == I think either the author is not very knowledgeable of js, or there must be a special exception in play here where the behavior of == is actually desired.
There was a problem hiding this comment.
It was not really my intention to start a discussion here :/
@gaye just allow for == in JSHint, so I can merge this PR.
There was a problem hiding this comment.
@mikedeboer Done!
| @@ -172,7 +176,7 @@ var jsDAV_CalendarQueryValidator = module.exports = Base.extend({ | |||
| component = component.getValue(); | |||
|
|
|||
| var isMatching = Util.textMatch(component, textMatch.value, textMatch["match-type"]); | |||
| return (textMatch["negate-condition"] ^ isMatching); | |||
| return (textMatch["negate-condition"] !== isMatching); | |||
There was a problem hiding this comment.
I'm not sure if this will yield the same results... did you check that?
|
Sorry if I sound like a jerk, but could you also just revert the Everything else, except the question about the bitwise operator change, looks good to me! Thanks for working on this! |
