FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: auto fix custom eslint rule for prefer-assert-methods.js by shobhitchittora · Pull Request #16652 · nodejs/node · GitHub

/ node Public

tools: auto fix custom eslint rule for prefer-assert-methods.js - #16652

Closed
shobhitchittora wants to merge 2 commits into
nodejs:masterfrom
shobhitchittora:pref-assert-methods-eslint-fixer
ConversationCommits (2)ChecksFiles changed
Closed

tools: auto fix custom eslint rule for prefer-assert-methods.js#16652
shobhitchittora wants to merge 2 commits into
nodejs:masterfrom
shobhitchittora:pref-assert-methods-eslint-fixer

Conversation

shobhitchittora commented Oct 31, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

This adds eslint fixer for auto-fixing the usage of assert operators. Also adding fileoverview for the perfer-assert-methods.js file.

For example the fixer change this assert(obj.value !== 9); to assert.notStrictEqual(obj.value, 9);

Refs: #16636

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
  • modify tests
Affected core subsystem(s)

Tools

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 31, 2017

shobhitchittora commented Nov 2, 2017

Copy link
Copy Markdown
Contributor Author

Hi @apapirovski. Can you also review this one? Thanks in advance.

BridgeAR approved these changes Nov 23, 2017
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In general LGTM but it should be extended to assert.ok() as assert() is actually just the alias.

addaleax commented Nov 30, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-commit/14472/

apapirovski commented Dec 9, 2017

Copy link
Copy Markdown
Contributor

ping @shobhitchittora — would you like to follow up on this? There's a bit of feedback here from @BridgeAR.

apapirovski added the wip Issues and PRs that are still a work in progress. label Dec 9, 2017

shobhitchittora commented Dec 10, 2017

Copy link
Copy Markdown
Contributor Author

@apapirovski @BridgeAR extended for assert.ok. Please review.

BridgeAR reviewed Dec 10, 2017
View reviewed changes
Comment thread test/parallel/test-eslint-prefer-assert-methods.js Outdated

BridgeAR Dec 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would not prefer assert.ok over assert. Both should be fine out of my perspective.

shobhitchittora Dec 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm a bit confused here. What did you mean when you said extend for assert.ok()?

BridgeAR Dec 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In general assert() should be treated identical to assert.ok(). And I meant the tests should be extended to test for both. Before I commented there were only tests for assert().

shobhitchittora Dec 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for clarifying this. I'll revert the added invalid test for assert(val).

shobhitchittora Dec 10, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

shobhitchittora force-pushed the pref-assert-methods-eslint-fixer branch from 246c566 to 74b89ca Compare December 10, 2017 06:41
apapirovski reviewed Dec 10, 2017
View reviewed changes
Comment thread test/parallel/test-eslint-prefer-assert-methods.js Outdated

apapirovski Dec 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I could be wrong but I think the idea was that assert(foo != bar) should yield the same as assert.ok(foo != bar). Since the former is an alias for the latter. That is, they should both report an error.

(I realize that might be modifying the current rule and is somewhat outside of the scope of the original work.)

BridgeAR Dec 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That would indeed be nice but I guess it is best to keep that for a separate PR and I am actually about to improve the assert message for cases like that in #17581

tools: auto fix custom eslint rule for prefer-assert-methods.js
16e64b5 
1. Extends tests
2. Refactors code
3. Adds fixer

Refs: nodejs#16636
shobhitchittora force-pushed the pref-assert-methods-eslint-fixer branch from 74b89ca to 16e64b5 Compare December 15, 2017 08:11

shobhitchittora commented Dec 15, 2017

Copy link
Copy Markdown
Contributor Author

@BridgeAR @apapirovski Updated the PR as per the new implementation by @cjihrig.

BridgeAR commented Jan 19, 2018

Copy link
Copy Markdown
Member

Mini-CI (enough for this test): https://ci.nodejs.org/job/node-test-commit-light/149/

BridgeAR added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Jan 31, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
tools: auto fix custom eslint rule
2d6912a 
1. Extends tests
2. Refactors code
3. Adds fixer

Refs: nodejs#16636

PR-URL: nodejs#16652
Refs: nodejs#16636
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

BridgeAR commented Feb 1, 2018

Copy link
Copy Markdown
Member

Landed in 2d6912a

BridgeAR closed this Feb 1, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
tools: auto fix custom eslint rule
3a15d71 
1. Extends tests
2. Refactors code
3. Adds fixer

Refs: #16636

PR-URL: #16652
Refs: #16636
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
tools: auto fix custom eslint rule
60ee303 
1. Extends tests
2. Refactors code
3. Adds fixer

Refs: #16636

PR-URL: #16652
Refs: #16636
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
tools: auto fix custom eslint rule
4f4bfbe 
1. Extends tests
2. Refactors code
3. Adds fixer

Refs: #16636

PR-URL: #16652
Refs: #16636
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
tools: auto fix custom eslint rule
1f2269a 
1. Extends tests
2. Refactors code
3. Adds fixer

Refs: #16636

PR-URL: #16652
Refs: #16636
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
tools: auto fix custom eslint rule
4f1939b 
1. Extends tests
2. Refactors code
3. Adds fixer

Refs: #16636

PR-URL: #16652
Refs: #16636
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
tools: auto fix custom eslint rule
a8b5a96 
1. Extends tests
2. Refactors code
3. Adds fixer

Refs: #16636

PR-URL: #16652
Refs: #16636
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
tools: auto fix custom eslint rule
efea67c 
1. Extends tests
2. Refactors code
3. Adds fixer

Refs: nodejs#16636

PR-URL: nodejs#16652
Refs: nodejs#16636
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR approved these changes
addaleax addaleax approved these changes
+1 more reviewer
apapirovski apapirovski left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL