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

tools: switch from closure-linter to eslint #1539

Closed
wants to merge 2 commits into from
Closed

tools: switch from closure-linter to eslint #1539

wants to merge 2 commits into from

Conversation

yosuke-furukawa
Copy link
Member

As we discussed before #1253, I would like to propose to switch from closure-linter to other ES6 friendly linter.

Because closure-linter does not recognize template string literals, class syntax and arrow functions, etc...
I have a motivation to improve our code to ES6. closure linter may be a blocker.

Currently, I just switched the closure linter to eslint and jscs.
And I fixed eslint errors in our existing JS files without test folder.

If you have any concerns, please tell me.

Reviewer: @chrisdickinson
Reviewer: @silverwind

@@ -65,15 +65,13 @@ function Protocol() {
}
exports.Protocol = Protocol;


Copy link
Contributor

Choose a reason for hiding this comment

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

Style guide is to put two blank newlines between top-level statements.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the current linter enforces this, but if possible, I'd loosen that rule to permit double empty lines in jscs. Otherwise, I'd be fine if we drop that requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks. two blank lines between functions is useful.

@chrisdickinson
Copy link
Contributor

Would it be possible to re-cut these commits into commits per-lib-file, one commit removing gjslint, one commit adding eslint, and one commit adding the eslint config? My computer can't handle showing the diff right now.

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Apr 27, 2015
@@ -394,12 +393,12 @@ function setupChannel(target, channel) {
} else if (handle instanceof UDP) {
message.type = 'dgram.Native';
} else {
throw new TypeError("This handle type can't be sent");
throw new TypeError(`This handle type can't be sent`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd allow this through "quotes": [2, "single", "avoid-escape"]

@silverwind
Copy link
Contributor

I think generally we should keep unused args where possible, if just for future reference. I'd suggest

"no-unused-vars": [2, {"vars": "all", "args": "none"}]

For the DTRACE vars, I think we could create a custom rule. I'd be vary of template strings in performance-critical parts, I think they aren't optimized as well as regular string concatenation.

Overall I like it as a first step, we can use this as a base to further tighten the rules later.

@yosuke-furukawa
Copy link
Member Author

@chrisdickinson @silverwind Thanks for your review.
I fixes the following points.

But I have not fixed the two blank newlines between top-level statements.

I would like to discuss the rules.

two blank newlines

Unfortunately, I could not find the "two blank newlines between top-level statements" on current jscs rules. And our current gjslint does not permit the rule.

If we don't have the interest to permit the rule, we set disallowMultipleLineBreaks: null in .jscsrc.
But current google javascript style guide in jscs has the rule (https://github.com/jscs-dev/node-jscs/blob/master/presets/google.json#L62) .

IMO, I would like to change the rule "two blank newlines" to "single newline". If so, no need to change current commits. :)

@silverwind
Copy link
Contributor

I'd be fine with single newline, I love compact code.

@yosuke-furukawa
Copy link
Member Author

@silverwind Thanks!

And we also discuss about the ES6 rules.
current rule:

"ecmaFeatures": {
    "blockBindings": true,
    "templateStrings": true,
    "octalLiterals": true,
    "binaryLiterals": true
},

blockBindings and octalLiterals and binaryLiterals are already used in current code.
I would like to add templateStrings. templateStrings does not have an impact on performance.
That is the performance result. #691 (comment)

And I just use templateStrings on some exception messages.

@silverwind
Copy link
Contributor

Would like a quick TC consensus whether we should drop or keep the 'two empty lines' rule between top-level 'blocks' in JavaScript files.

Me and @yosuke-furukawa would like to drop that rule.

@@ -571,7 +567,8 @@ REPLServer.prototype.complete = function(line, callback) {
this.context.constructor &&
this.context.constructor.name === 'Context') {
var contextProto = this.context;
while (contextProto = Object.getPrototypeOf(contextProto)) {
while (
(contextProto = Object.getPrototypeOf(contextProto)) === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

() === true is unnecesary

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO, we should use condition in conditional sentences.
I am happy if linter checks the following bug.

if (a = 'test') { // a == 'test' is really needed code 
  // ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, while assignment inside a condition is a common error, I like the conciseness of how it was before.

Copy link
Contributor

Choose a reason for hiding this comment

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

The common practice for those assignments is to write them like this:

// good code
if ((a = 'test')) {
  // ...
}

So additional parentheses are a signal to a linter that a developer is intended to write this code as is.

But this is very verbose and completely unnecessary:

// don't do this
if ((a = 'test') === true) {
  // ...
}

@Fishrock123
Copy link
Contributor

I am +1 on moving to a more modern linter.

I am very -1 on the current state of style changes. This is currently far too strict.

(Also more than one-line-spacing shouldn't matter to the linter..)

Protocol.prototype._newRes = function(raw) {
this.res = { raw: raw || '', headers: {} };
this.res = {raw: raw || '', headers: {}};
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it's more common style to leave blank space on { }. Could we leave that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, looks subjectively cleaner with the spaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. We should change disallowSpacesInsideObjectBrackets on .jscsrc .

@rlidwka
Copy link
Contributor

rlidwka commented Apr 30, 2015

I am very -1 on the current state of style changes. This is currently far too strict.

Same thought here. This PR is about changing linter, not changing code style.

If eslint/jscs can't validate the existing code style, it is not a good reason to change the existing code (you know, git blame worsening and stuff). In that case maybe we should stay on gjslint instead.

I don't have much of experience with jscs, but I could say a lot about eslint. It is good, but it is unacceptably opinionated by default. Please use --reset flag and enable only those rules that you need here. In particular, if we have empty blocks (and those are reasonable to have), don't enable the rule that complains about them.

@silverwind
Copy link
Contributor

Agreed, we should first get the linter in with the minimal necessary changes. Further changes to the rules can be discussed in seperate PRs.

@rlidwka
Copy link
Contributor

rlidwka commented Apr 30, 2015

Maybe enable only a minimal subset of eslint rules first (I personally use only 11 rules that detect most of the errors). After that we could discuss each and every rule you want to enable on top of that in separate PRs.

@yosuke-furukawa
Copy link
Member Author

Agreed. I should separate the eslint/jscs change request and code style change request.
code style discussion is not single topic, I will recut this pull request.
Thank you for your suggestion.

@yosuke-furukawa
Copy link
Member Author

@chrisdickinson @silverwind @Fishrock123 @trevnorris @rlidwka

I loosen the eslint and jscs rules and I applied the minimum rule sets.
So current rules don't permit double lines / yoda condition / assignment in conditional statement.

I just use semicolon, comma, comment rules.

please see the following eslint / jscs rules.

LGTY????

@rlidwka
Copy link
Contributor

rlidwka commented May 1, 2015

I don't like that list of disabled rules in eslint (https://github.com/yosuke-furukawa/io.js/commit/4a9f61a4956a5032fd0f2d0177ecc6f11fc91832). It's not clear which rules we do use. And also if eslint removes one of the disabled rules in the future, this config will throw an error.

Maybe use --reset and explicitly enable all the rules we want?

I'm not familiar with jscs to say anything, but I heard eslint already has most of jscs rules. What do we need two separate linters for?

@yosuke-furukawa yosuke-furukawa changed the title tools: switch from closure-linter to eslint and jscs tools: switch from closure-linter to eslint May 3, 2015
@silverwind
Copy link
Contributor

@yosuke-furukawa can you rebase please? The recent revert of url causes the patch to fail. I expect to LGTM this shortly.

@yosuke-furukawa
Copy link
Member Author

@silverwind Done. But If this pull request merged, I will re-rebase them. #1650

@silverwind
Copy link
Contributor

LGTM

@Fishrock123
Copy link
Contributor

I'm ok with these style fixes.

@iojs/collaborators ptal

@indutny
Copy link
Member

indutny commented May 7, 2015

I'm fine with this! LGTM

@jbergstroem
Copy link
Member

LGTM

@trevnorris
Copy link
Contributor

Great job keeping it as close to how things are currently done. LGTM

@yosuke-furukawa
Copy link
Member Author

Thank you for your review! I would like to push the changes.
But, current commit logs are a little messy.

I will squash my commits to 3 commits like:

  • Switch closure linter to eslint
  • Add eslint settings
  • Fix styles on library

Is that OK??? Does anyone have some suggestions ?

@silverwind
Copy link
Contributor

I'd do two commits, one for the whole of eslint, including settings, the other for style fixes.

@yosuke-furukawa
Copy link
Member Author

@silverwind No problem!

PR-URL: #1539
Fixes: #1253
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
PR-URL: #1539
Fixes: #1253
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
yosuke-furukawa added a commit that referenced this pull request May 9, 2015
PR-URL: #1539
Fixes: #1253
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
yosuke-furukawa added a commit that referenced this pull request May 9, 2015
PR-URL: #1539
Fixes: #1253
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@yosuke-furukawa
Copy link
Member Author

Thank you all reviewer, Landed in f9dd34d , 19ffb5c .

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
PR-URL: nodejs#1539
Fixes: nodejs#1253
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
PR-URL: nodejs#1539
Fixes: nodejs#1253
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
pmq20 added a commit to pmq20/node that referenced this pull request Sep 23, 2015
targos pushed a commit that referenced this pull request Sep 23, 2015
Ref: #1539
PR-URL: #3018
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yosuke Furukawa <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Sep 25, 2015
Ref: #1539
PR-URL: #3018
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yosuke Furukawa <[email protected]>
@watilde watilde mentioned this pull request Nov 1, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants