Skip to content
This repository has been archived by the owner on Nov 27, 2019. It is now read-only.

chore: Add ESLint no-console and max-len rules #587

Merged
merged 2 commits into from
Sep 22, 2016
Merged

chore: Add ESLint no-console and max-len rules #587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29b0ea2
chore: Add ESLint no-console and max-len rules
pdehaan Sep 21, 2016
518ec3a
chore: Switch from console.log to logger.info
pdehaan Sep 22, 2016
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
5 changes: 3 additions & 2 deletions .eslintrc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,16 @@ rules:
eqeqeq: error
indent: [error, 2, {SwitchCase: 1}]
key-spacing: [error, {beforeColon: false, afterColon: true}]
max-len: [off, 80] # TODO: Set to warn|error?
max-len: [error, 80, {ignoreComments: true}]
new-cap: error
no-console: off # TODO: Set to warn|error?
no-console: error
no-mixed-spaces-and-tabs: error
no-multi-str: error
no-multiple-empty-lines: error
no-spaced-func: error
no-trailing-spaces: error
no-unused-vars: [warn, {vars: all, args: none}]
no-warning-comments: warn
no-with: error
one-var: [error, never]
operator-linebreak: [error, after]
Expand Down
2 changes: 1 addition & 1 deletion lib/firefox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ var GARBAGE = [
/\[JavaScript Warning: "TypeError: "[\w\d]+" is read-only"\]/,
/JavaScript strict warning: /,
/\#\#\#\!\!\! \[Child\]\[DispatchAsyncMessage\]/,
/JavaScript strict warning: resource:\/\/\/modules\/sessionstore\/SessionStore\.jsm/,
/JavaScript strict warning: resource:\/\/\/modules\/sessionstore\/SessionStore\.jsm/, // eslint-disable-line max-len
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be concatenated and broken into separate lines like /first/ + /second/ ? That might be harder to read, I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you concatenate Regular Expressions?
Do half these even need to be regular expressions vs plain strings?

Copy link
Contributor

Choose a reason for hiding this comment

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

ha, well, it's legacy cruft for sure. I wouldn't worry about it too much, eslint disable comments are fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I didn't feel like putting in TOO much effort on this, since it seems like ESLint has a fix in progress for ignoring long regular expressions.

/JavaScript strict warning: resource:\/\/gre\/components\/nsSearchService\.js/
];

Expand Down
13 changes: 9 additions & 4 deletions lib/post.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ var fs = require("fs");
var url = require("url");
var net = require("net");
var when = require("when");
var utils = require("./utils");
var logger = utils.console;

function postXPI(manifest, options) {
var postURL = options.postUrl;
Expand Down Expand Up @@ -40,16 +42,19 @@ function postXPI(manifest, options) {
reject(e);
});
client.on("close", function(hadError) {
console.log(hadError ? "Posting XPI to %s failed" :
"Posted XPI to %s", postURL);
if (hadError) {
logger.error("Posting XPI to " + postURL + " failed");
} else {
logger.info("Posted XPI to " + postURL);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I guess I should also call out that I slightly changed the logic here, where I converted this to an if..else instead of a ternery statement, because it let me use logger.error() and logger.info() a bit more appropriately [said the guy with zero context].

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that, looks good

fs.unlink(xpiPath);
console.log("Removed XPI from " + xpiPath);
logger.info("Removed XPI from " + xpiPath);
resolve();
});
});
});
}).then(null, function(err) {
console.error(err);
console.error(err); // eslint-disable-line no-console
});
}
module.exports = postXPI;
2 changes: 1 addition & 1 deletion lib/rdf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ var GUIDS = require("./settings").ids;
var MIN_VERSION = require("./settings").MIN_VERSION;
var MAX_VERSION = require("./settings").MAX_VERSION;

var UUID_PATTERN = /^\{([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})\}$/;
var UUID_PATTERN = /^\{([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})\}$/; // eslint-disable-line max-len

// Copied from mozilla-central/addon-sdk/source/lib/sdk/bootstrap.js
function realDomain(id) {
Expand Down
8 changes: 4 additions & 4 deletions lib/sign.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ function sign(options, config) {
}
});
if (missingOptions.length) {
console.error();
console.error(); // eslint-disable-line no-console
Copy link
Contributor

Choose a reason for hiding this comment

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

I think all these do need to be direct console calls. I forget why exactly but there was some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only reason I can think of was to add some extra leading/trailing whitespace in the console.
I was trying to figure out how to wrap this in a single console.error() call, but it was a confusing mix of:

var errors = missingOptions.map(function(flag) {
  return `  error: missing required option '${flag}'`;
});
console.error(`\n${errors.join("\n")}\n`); // eslint-disable-line no-console

And I wasn't convinced that my theory would be any cleaner or clearer.

missingOptions.forEach(function(flag) {
console.error(" error: missing required option `%s'", flag);
console.error(" error: missing required option `%s'", flag); // eslint-disable-line no-console
});
console.error();
console.error(); // eslint-disable-line no-console
return reject();
}

Expand Down Expand Up @@ -145,7 +145,7 @@ function signCmd(program, options, config) {
}).catch(function(err) {
logger.error("FAIL");
if (err) {
console.error(err.stack);
console.error(err.stack); // eslint-disable-line no-console
}
config.systemProcess.exit(1);
});
Expand Down
2 changes: 1 addition & 1 deletion lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ function log(type) {
var first = "JPM [" + type + "] " + (messages.shift() + "");

if (process.env.NODE_ENV !== "test") {
console.log.apply(console, [first].concat(messages));
console.log.apply(console, [first].concat(messages)); // eslint-disable-line no-console
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/documentation/test.docs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe("Spell Checking", function() {

if (results.length > 0) {
// prints all the teacher results (even the ignored suggestions)
console.log(JSON.stringify(results, null, 2));
console.log(JSON.stringify(results, null, 2)); // eslint-disable-line no-console
}

// filter out results with descriptions which we do not want to consider as errors
Expand Down
2 changes: 1 addition & 1 deletion test/functional/test.run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ describe("jpm run", function() {
json = JSON.parse(data);
}
catch (e) {
console.log("Something is wrong with output:\n" + output);
console.log("Something is wrong with output:\n" + output); // eslint-disable-line no-console
}
return json;
}
Expand Down