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

test,tools: limit lint tolerance of gc global by Trott · Pull Request #6324 · nodejs/node · GitHub

/ node Public

test,tools: limit lint tolerance of gc global - #6324

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:gc
ConversationCommits (2)ChecksFiles changed
Closed

test,tools: limit lint tolerance of gc global#6324
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:gc

Conversation

Trott commented Apr 21, 2016

Copy link
Copy Markdown
Member
Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test tools

Description of change

Lint rules permitted the gc global in any test file. This change
limits it to just the files that need it.

Trott added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. lts-watch-v4.x labels Apr 21, 2016

bnoordhuis commented Apr 21, 2016

Copy link
Copy Markdown
Member

LGTM, I guess. Would this allow global.gc() without an eslint directive?

cjihrig commented Apr 21, 2016

Copy link
Copy Markdown
Contributor

Would this allow global.gc() without an eslint directive?

It looks like it, based on the change to test/common.js, but I could be wrong.

LGTM

jasnell commented Apr 21, 2016

Copy link
Copy Markdown
Member

This is fine and the change LGTM but I'm curious.. why?

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

Trott commented Apr 27, 2016

Copy link
Copy Markdown
Member Author

@jasnell wrote:

This is fine and the change LGTM but I'm curious.. why?

I prefer exceptions to lint rules to be applied only as narrowly as necessary. So rather than applying an exception to the 1324 JS files in test, this applies that exception to just the dozen or so files that need it.

Trott commented Apr 27, 2016

Copy link
Copy Markdown
Member Author

@bnoordhuis asked:

Would this allow global.gc() without an eslint directive?

Yes, that would pass linting.

addaleax commented Apr 28, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell commented Apr 28, 2016

Copy link
Copy Markdown
Member

@Trott ... ok :-) thanks!

bnoordhuis commented Apr 28, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Anyone prefer gc() over global.gc() or vice versa? I have a mild preference for the latter because it lets us drop the eslint directive. It's mostly aesthetic though.

cjihrig commented Apr 28, 2016

Copy link
Copy Markdown
Contributor

Dropping the eslint directive is preferably IMO in case things change in the future on their side or ours.

Trott added 2 commits April 28, 2016 10:20
test,tools: limit lint tolerance of gc global
c91423f 
Lint rules permitted the `gc` global in any test file. This change
limits it to just the files that need it.
squash me
145005a

Trott commented Apr 28, 2016

Copy link
Copy Markdown
Member Author

Removed the directives and replaced instances of gc with global.gc. PTAL!

cjihrig commented Apr 28, 2016

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment

bnoordhuis commented Apr 28, 2016

Copy link
Copy Markdown
Member

LGTM

Trott commented Apr 28, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/2422/

Trott commented Apr 29, 2016

Copy link
Copy Markdown
Member Author

CI is green except for unrelated and known-problematic tick processor test on OS X which will be fixed Very Soon.

Trott added a commit to Trott/io.js that referenced this pull request Apr 29, 2016
test,tools: limit lint tolerance of gc global
6c49714 
Lint rules permitted the `gc` global in any test file. This change
limits it to just the files that need it.

PR-URL: nodejs#6324
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Trott commented Apr 29, 2016

Copy link
Copy Markdown
Member Author

Landed in 6c49714.

Trott closed this Apr 29, 2016
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
test,tools: limit lint tolerance of gc global
089362f 
Lint rules permitted the `gc` global in any test file. This change
limits it to just the files that need it.

PR-URL: #6324
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This was referenced May 4, 2016
Closed
Merged

MylesBorins commented May 18, 2016

Copy link
Copy Markdown
Contributor

not landing cleanly @Trott feel free to tag don't land if you don't want to backport

bnoordhuis commented May 19, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

@thealphanerd @Trott I think this can land on v4.x once #6871 lands.

Trott deleted the gc branch January 13, 2022 22:43
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

No reviews

Assignees

No one assigned

Labels

test Issues and PRs related to the tests. 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