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

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

Closed
wants to merge 2 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 21, 2016

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 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
Copy link
Member

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

@cjihrig
Copy link
Contributor

cjihrig commented Apr 21, 2016

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
Copy link
Member

jasnell commented Apr 21, 2016

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

@Trott
Copy link
Member Author

Trott commented Apr 27, 2016

@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
Copy link
Member Author

Trott commented Apr 27, 2016

@bnoordhuis asked:

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

Yes, that would pass linting.

@addaleax
Copy link
Member

LGTM

@jasnell
Copy link
Member

jasnell commented Apr 28, 2016

@Trott ... ok :-) thanks!

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 28, 2016

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
Copy link
Contributor

cjihrig commented Apr 28, 2016

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

Lint rules permitted the `gc` global in any test file. This change
limits it to just the files that need it.
@Trott
Copy link
Member Author

Trott commented Apr 28, 2016

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

@cjihrig
Copy link
Contributor

cjihrig commented Apr 28, 2016

LGTM

1 similar comment
@bnoordhuis
Copy link
Member

LGTM

@Trott
Copy link
Member Author

Trott commented Apr 28, 2016

@Trott
Copy link
Member Author

Trott commented Apr 29, 2016

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
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@Trott
Copy link
Member Author

Trott commented Apr 29, 2016

Landed in 6c49714.

@Trott Trott closed this Apr 29, 2016
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins
Copy link
Contributor

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

@bnoordhuis
Copy link
Member

bnoordhuis commented May 19, 2016

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

@Trott Trott deleted the gc branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants