Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use jshint, clean up tests r=mikedeboer #106

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .jshintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
examples/data/addressbook/redis.js
lib/CardDAV/**/*
lib/DAV/**/*
lib/DAVACL/**/*
lib/shared/**/*
lib/VObject/**/*
test/**/*
73 changes: 73 additions & 0 deletions .jshintrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
{
"bitwise" : true, // true: Prohibit bitwise operators (&, |, ^, etc.)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why you'd want to prohibit this...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bitwise operators are very "low-level". You shouldn't be thinking in terms of them, ideally. In programming, you should strive to describe what you would like done and not how to do it. There's an interesting, somewhat tangential discussion here http://latentflip.com/imperative-vs-declarative/

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I don't think this is used in jsDAV, I'm fine with it. As I'm a low-level kind of programmer, mixing my days with C, Obj-C, JS and other things, I don't really see a problem... it's just a bit of basic CS.

But, if it helps contributors write better code, it'll be worth it ;)

"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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please set this to false

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a best practice not to.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not really my intention to start a discussion here :/

@gaye just allow for == in JSHint, so I can merge this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikedeboer Done!

"forin" : false, // true: Require filtering for..in loops with obj.hasOwnProperty()
"immed" : false, // true: Require immediate invocations to be wrapped in parens e.g. `(function () { } ());`
"indent" : 4, // {int} Number of spaces to use for indentation
"latedef" : false, // true: Require variables/functions to be defined before being used
"newcap" : true, // true: Require capitalization of all constructor functions e.g. `new F()`
"noarg" : true, // true: Prohibit use of `arguments.caller` and `arguments.callee`
"noempty" : true, // true: Prohibit use of empty blocks
"nonew" : true, // true: Prohibit use of constructors for side-effects (without assignment)
"plusplus" : false, // true: Prohibit use of `++` & `--`
"quotmark" : "double", // Quotation mark consistency:

"undef" : true, // true: Require all non-global variables to be declared (prevents global leaks)
"unused" : true, // true: Require all defined variables be used
"strict" : true, // true: Requires all functions run in ES5 Strict Mode
"trailing" : true, // true: Prohibit trailing whitespaces
"maxparams" : false, // {int} Max number of formal params allowed per function
"maxdepth" : false, // {int} Max depth of nested blocks (within functions)
"maxstatements" : false, // {int} Max number statements per function
"maxcomplexity" : false, // {int} Max cyclomatic complexity per function
"maxlen" : 100, // {int} Max number of characters per line

"asi" : false, // true: Tolerate Automatic Semicolon Insertion (no semicolons)
"boss" : false, // true: Tolerate assignments where comparisons would be expected
"debug" : false, // true: Allow debugger statements e.g. browser breakpoints.
"eqnull" : false, // true: Tolerate use of `== null`
"esnext" : true, // true: Allow ES.next (ES6) syntax (ex: `const`)
"moz" : false, // true: Allow Mozilla specific syntax (extends and overrides esnext features)
// (ex: `for each`, multiple try/catch, function expression…)
"evil" : false, // true: Tolerate use of `eval` and `new Function()`
"expr" : false, // true: Tolerate `ExpressionStatement` as Programs
"funcscope" : false, // true: Tolerate defining variables inside control statements
"globalstrict" : false, // true: Allow global "use strict" (also enables 'strict')
"iterator" : false, // true: Tolerate using the `__iterator__` property
"lastsemic" : false, // true: Tolerate omitting a semicolon for the last statement of a 1-line block
"laxbreak" : false, // true: Tolerate possibly unsafe line breakings
"laxcomma" : false, // true: Tolerate comma-first style coding
"loopfunc" : false, // true: Tolerate functions being defined in loops
"multistr" : false, // true: Tolerate multi-line strings
"proto" : false, // true: Tolerate using the `__proto__` property
"scripturl" : false, // true: Tolerate script-targeted URLs
"smarttabs" : false, // true: Tolerate mixed tabs/spaces when used for alignment
"shadow" : false, // true: Allows re-define variables later in code e.g. `var x=1; x=2;`
"sub" : false, // true: Tolerate using `[]` notation when it can still be expressed in dot notation
"supernew" : false, // true: Tolerate `new function () { ... };` and `new Object;`
"validthis" : false, // true: Tolerate using this in a non-constructor function

"browser" : false, // Web Browser (window, document, etc)
"couch" : false, // CouchDB
"devel" : false, // Development/debugging (alert, confirm, etc)
"dojo" : false, // Dojo Toolkit
"jquery" : false, // jQuery
"mootools" : false, // MooTools
"node" : true, // Node.js
"nonstandard" : false, // Widely adopted globals (escape, unescape, etc)
"prototypejs" : false, // Prototype and Scriptaculous
"rhino" : false, // Rhino
"worker" : false, // Web Workers
"wsh" : false, // Windows Scripting Host
"yui" : false, // Yahoo User Interface

// Legacy
"nomen" : false, // true: Prohibit dangling `_` in variables
"onevar" : false, // true: Allow only one `var` statement per function
"passfail" : false, // true: Stop on first error
"white" : false, // true: Check against strict whitespace and indentation rules

"globals" : {}
}
4 changes: 4 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
language: node_js
node_js:
- "0.8"
- "0.10"
20 changes: 20 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
.PHONY: default
default: lint test

.PHONY: lint
lint: node_modules
./node_modules/.bin/jshint examples/ lib/ test/

node_modules:
npm install

.PHONY: test
test: node_modules test-mocha
node test/test_base.js

# TODO: test/test_ftp.js and test/test_xml.js need to be resurrected.
.PHONY: test-mocha
test-mocha: node_modules
./node_modules/.bin/mocha \
test/test_codesearch.js \
test/test_filelist.js
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# jsDAV

[![Build Status](https://travis-ci.org/mikedeboer/jsdav.png?branch=master)](https://travis-ci.org/mikedeboer/jsdav)

jsDAV allows you to easily add WebDAV support to a NodeJS application.
jsDAV is meant to cover the entire standard, and attempts to allow integration using an easy to understand API.

Expand Down Expand Up @@ -54,5 +56,9 @@ improvements and bugfixes, to see if they can be ported to jsDAV.
See the [wiki](https://github.com/mikedeboer/jsDAV/wiki) for more information or
ask a question in the [mailing list](https://groups.google.com/d/forum/jsdav)!

## Testing

In order to run the test suite locally, you can run `npm test`. In addition to the mocha suite, there are also "smoke tests" which can be run manually in `test/smoke`.


Amsterdam, 2010. Mike de Boer.
4 changes: 2 additions & 2 deletions examples/addressbookserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ var DB_ARGS = {};
host: "localhost",
db: "jsdav",
port: 27017,
//username: "", //optional, if both username and password are provided, authentication will be performed before returning connection
//password: "" //see above
//username: "", // optional, will try auth if both username and password are provided
//password: "" // see above
};
*/

Expand Down
2 changes: 1 addition & 1 deletion examples/data/addressbook/mongo.js
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ===?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I make a point of not knowing how == and != coerce things so that I can use my brain for other, more useful activities

coll.ensureIndex(op.data, {unique: true}, next);
else
coll.insert(op.data, next);
Expand Down
2 changes: 1 addition & 1 deletion lib/CalDAV/calendar.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ var Exc = require("./../shared/exceptions");
*
* The Calendar can contain multiple calendar objects
*/
var jsCalDAV_Calendar = module.exports = jsDAV_Collection.extend(jsCalDAV_iCalendar, jsDAV_iProperties, jsDAVACL_iAcl, {
module.exports = jsDAV_Collection.extend(jsCalDAV_iCalendar, jsDAV_iProperties, jsDAVACL_iAcl, {
/**
* This is an array with calendar information
*
Expand Down
40 changes: 22 additions & 18 deletions lib/CalDAV/calendarObject.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@

var jsDAV_File = require("./../DAV/file");
var jsCalDAV_iCalendarObject = require("./interfaces/iCalendarObject");
var jsDAVACL_iAcl = require("./../DAVACL/interfaces/iAcl")
var jsDAVACL_iAcl = require("./../DAVACL/interfaces/iAcl");

var Exc = require("./../shared/exceptions");
var Util = require("./../shared/util");

/**
* The Calendar object represents a single Calendar Object from a calendar
*/
var jsCalDAV_Card = module.exports = jsDAV_File.extend(jsCalDAV_iCalendarObject, jsDAVACL_iAcl, {
module.exports = jsDAV_File.extend(jsCalDAV_iCalendarObject, jsDAVACL_iAcl, {
/**
* CalDav backend
*
Expand Down Expand Up @@ -68,18 +68,20 @@ var jsCalDAV_Card = module.exports = jsDAV_File.extend(jsCalDAV_iCalendarObject,
*/
get: function(callback) {
if (this.calData.calendardata)
return callback(null, this.calData.calendardata)
return callback(null, this.calData.calendardata);

// Pre-populating 'calendardata' is optional. If we don't yet have it
// already, we fetch it from the backend.
var self = this;
this.caldavBackend.getCalendarObject(this.calendarInfo.id, this.calData.uri, function(err, calData) {
if (err)
return callback(err);
this.caldavBackend.getCalendarObject(
this.calendarInfo.id, this.calData.uri, function(err, calData) {
if (err)
return callback(err);

self.calData.calendardata = calData;
callback(null, self.calData.calendardata);
});
self.calData.calendardata = calData;
callback(null, self.calData.calendardata);
}
);
},

/**
Expand All @@ -92,14 +94,16 @@ var jsCalDAV_Card = module.exports = jsDAV_File.extend(jsCalDAV_iCalendarObject,
var self = this;
if (Buffer.isBuffer(calData))
calData = calData.toString("utf8");
this.caldavBackend.updateCalendarObject(this.calendarInfo.id, this.calData.uri, calData, function(err, etag) {
if (err)
return callback(err);

self.calData.calendardata = calData;
self.calData.etag = etag;
callback(null, etag);
});
this.caldavBackend.updateCalendarObject(
this.calendarInfo.id, this.calData.uri, calData, function(err, etag) {
if (err)
return callback(err);

self.calData.calendardata = calData;
self.calData.etag = etag;
callback(null, etag);
}
);
},

/**
Expand Down Expand Up @@ -132,7 +136,7 @@ var jsCalDAV_Card = module.exports = jsDAV_File.extend(jsCalDAV_iCalendarObject,
this.get(function(err, data) {
if (err)
return callback(err);
callback(null, '"' + Util.createHash(data) + '"');
callback(null, "\"" + Util.createHash(data) + "\"");
});
},

Expand Down
46 changes: 23 additions & 23 deletions lib/CalDAV/calendarQueryParser.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
"use strict";

var Base = require("./../shared/base");
var Util = require("./../shared/util");
var Exc = require("./../shared/exceptions");
var Xml = require("./../shared/xml");
var xpath = require("xpath");
Expand All @@ -33,7 +32,7 @@ function xSelect(expr, doc, single) {

for (var fullNs in Xml.xmlNamespaces) {
if (Xml.xmlNamespaces.hasOwnProperty(fullNs)) {
if (Xml.xmlNamespaces[fullNs] == short) {
if (Xml.xmlNamespaces[fullNs] === short) {
revmap[short] = fullNs;
return fullNs;
}
Expand All @@ -46,25 +45,25 @@ function xSelect(expr, doc, single) {
var type = XPathResult.ANY_TYPE;

var result = expression.evaluate(doc, type, null);
if (result.resultType == XPathResult.STRING_TYPE) {
result = result.stringValue;
}
else if (result.resultType == XPathResult.NUMBER_TYPE) {
result = result.numberValue;
}
else if (result.resultType == XPathResult.BOOLEAN_TYPE) {
result = result.booleanValue;
}
else {
result = result.nodes;
if (single)
result = result[0];
switch (result.resultType) {
case XPathResult.STRING_TYPE:
result = result.stringValue;
break;
case XPathResult.NUMBER_TYPE:
result = result.numberValue;
break;
case XPathResult.BOOLEAN_TYPE:
result = result.booleanValue;
break;
default:
result = single ? result.nodes[0] : result.nodes;
break;
}

return result;
}

var jsCalDAV_CalendarQueryParser = module.exports = Base.extend({
module.exports = Base.extend({
dom: null,
filters: null,
requestedProperties: null,
Expand All @@ -91,13 +90,13 @@ var jsCalDAV_CalendarQueryParser = module.exports = Base.extend({
end = jsVObject_DateTimeParser.parseDateTime(end);

if (end <= start)
throw Exc.BadRequest("The end-date must be larger than the start-date in the expand element.");
throw Exc.BadRequest("The end-date must be larger than the start-date in the expand " +
"element.");

return {
"start": start,
"end": end
};

},

parse: function() {
Expand Down Expand Up @@ -142,7 +141,7 @@ var jsCalDAV_CalendarQueryParser = module.exports = Base.extend({

var textMatchNode = textMatchNodes[0];
var negateCondition = textMatchNode.getAttribute("negate-condition");
negateCondition = negateCondition == "yes";
negateCondition = negateCondition === "yes";
var collation = textMatchNode.getAttribute("collation");
if (!collation)
collation = "i;ascii-casemap";
Expand Down Expand Up @@ -187,7 +186,8 @@ var jsCalDAV_CalendarQueryParser = module.exports = Base.extend({
end = end ? jsVObject_DateTimeParser.parseDateTime(end) : null;

if (start && end && end <= start)
throw Exc.BadRequest("The end-date must be larger than the start-date in the time-range filter");
throw Exc.BadRequest("The end-date must be larger than the start-date in the " +
"time-range filter.");

return {
"start": start,
Expand All @@ -212,9 +212,9 @@ var jsCalDAV_CalendarQueryParser = module.exports = Base.extend({
"time-range": this._parseTimeRange(compFilterNode)
};

if (compFilter["time-range"] && !(compFilter["name"] in eventTypes)) {
throw Exc.BadRequest("The time-range filter is not defined for the "
+ compFilter["name"] + " component");
if ("time-range" in compFilter && !(compFilter.name in eventTypes)) {
throw Exc.BadRequest("The time-range filter is not defined for the " +
compFilter.name + " component");
}

result.push(compFilter);
Expand Down
Loading