chore: Add ESLint no-console and max-len rules #587
Conversation
|
Unfortunately there isn't currently a great way to have ESLint ignore really long regular expressions, so I had to use Once a fix for eslint/eslint#3229 lands, we can remove those shameful |
| @@ -40,16 +40,16 @@ function postXPI(manifest, options) { | |||
| reject(e); | |||
| }); | |||
| client.on("close", function(hadError) { | |||
| console.log(hadError ? "Posting XPI to %s failed" : | |||
| console.log(hadError ? "Posting XPI to %s failed" : // eslint-disable-line no-console | |||
There was a problem hiding this comment.
This looks a bit weird since the console.log() is wrapped on 2 lines, but I had to put the comment on the same line as opening console.log().
| @@ -40,16 +40,16 @@ function postXPI(manifest, options) { | |||
| reject(e); | |||
| }); | |||
| client.on("close", function(hadError) { | |||
| console.log(hadError ? "Posting XPI to %s failed" : | |||
| console.log(hadError ? "Posting XPI to %s failed" : // eslint-disable-line no-console | |||
There was a problem hiding this comment.
These should be switched to the logger. You can set it up like this at the top of the module:
var utils = require("./utils");
var logger = utils.console;
And use it like either logger.info(...) or logger.error(...)
| @@ -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 | |||
There was a problem hiding this comment.
Can these be concatenated and broken into separate lines like /first/ + /second/ ? That might be harder to read, I'm not sure.
There was a problem hiding this comment.
Can you concatenate Regular Expressions?
Do half these even need to be regular expressions vs plain strings?
There was a problem hiding this comment.
ha, well, it's legacy cruft for sure. I wouldn't worry about it too much, eslint disable comments are fine.
There was a problem hiding this comment.
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.
| @@ -57,11 +57,11 @@ function sign(options, config) { | |||
| } | |||
| }); | |||
| if (missingOptions.length) { | |||
| console.error(); | |||
| console.error(); // eslint-disable-line no-console | |||
There was a problem hiding this comment.
I think all these do need to be direct console calls. I forget why exactly but there was some reason.
There was a problem hiding this comment.
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.
| logger.error("Posting XPI to " + postURL + " failed"); | ||
| } else { | ||
| logger.info("Posted XPI to " + postURL); | ||
| } |
There was a problem hiding this comment.
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].
|
Thanks! |
Ref #585