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.

Sign up for GitHub

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: enable (sensible) additional eslint rules #16243

Closed
wants to merge 3 commits into from
Closed

tools: enable (sensible) additional eslint rules #16243

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b00944
test: disable accessor-pairs in test-util-inspect
apapirovski Oct 16, 2017
bda0695
lib: fix eslint no-lonely-if
apapirovski Oct 16, 2017
99fe59e
tools: enable additional eslint rules
apapirovski Oct 16, 2017
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
3 changes: 3 additions & 0 deletions test/parallel/test-util-inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ const JSStream = process.binding('js_stream').JSStream;
const util = require('util');
const vm = require('vm');

/* eslint-disable accessor-pairs */
Copy link
Contributor

Choose a reason for hiding this comment

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

Still a good idea to comment why:

// this test uses unmatched accessors to try and trip up `inspect`


assert.strictEqual(util.inspect(1), '1');
assert.strictEqual(util.inspect(false), 'false');
assert.strictEqual(util.inspect(''), "''");
Expand Down Expand Up @@ -1149,3 +1151,4 @@ if (typeof Symbol !== 'undefined') {
}

assert.doesNotThrow(() => util.inspect(process));
/* eslint-enable accessor-pairs */
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure, but this line seems not needed since there is no code left for linting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that was my thinking but I've noticed a bunch of our test files do this so I just followed the convention.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, maybe this is some over-caution for possible future code addition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. If someone does know, I would love to know the reason. Or maybe we're all just following the lead of the first person that did this... 😂

Copy link
Member

Choose a reason for hiding this comment

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

I think the main reason is to try to limit the deactivation for any rule to the minimum required lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, in many tests such enabling comments are inserted in the middle of the code. But as the last line, they may be a bit confusing. However, they can have the same role as trailing commas in arrays and objects: safety device for future additions.