GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: do not disable linter for entire files by aduh95 · Pull Request #48299 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 commented Jun 2, 2023

Copy link
Copy Markdown
Contributor

Disabling linter for single lines is less error prone, disabling on the entire file can swallow errors that are easier to miss during code reviews, when one would assume the rule is enabled.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Jun 2, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. web streams Issues and PRs related to the Web Streams API. labels Jun 2, 2023
targos
targos approved these changes Jun 2, 2023
View reviewed changes
debadree25
debadree25 approved these changes Jun 2, 2023
View reviewed changes
@debadree25 debadree25 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jun 2, 2023
@debadree25

debadree25 commented Jun 2, 2023

Copy link
Copy Markdown
Contributor

this probably doesnt need ci?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Jun 2, 2023

Copy link
Copy Markdown
Contributor Author

this probably doesnt need ci?

node/doc/contributing/collaborator-guide.md

Lines 207 to 217 in 260092e

Do not land any pull requests without the necessary passing CI runs.
A passing (green) GitHub Actions CI result is required. A passing (green or
yellow) [Jenkins CI](https://ci.nodejs.org/) is also required if the pull
request contains changes that will affect the `node` binary. This is because
GitHub Actions CI does not cover all the environments supported by Node.js.
<details>
<summary>Changes that affect the `node` binary</summary>
Changes in the following folders (except comment-only changes) are guaranteed to
affect the `node` binary:

That's correct, those are comment-only changes, GitHub CI is sufficient according to our policy.
debadree25 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added fast-track PRs proposed for a shorter-than-standard waiting period before landing. and removed needs-ci PRs that need a full CI run. labels Jun 2, 2023
@github-actions

github-actions Bot commented Jun 2, 2023

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @aduh95. Please 👍 to approve.
debadree25, MoLow, anonrig, and ShogunPanda reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow approved these changes Jun 2, 2023
View reviewed changes
anonrig
anonrig approved these changes Jun 2, 2023
View reviewed changes
ShogunPanda
ShogunPanda approved these changes Jun 2, 2023
View reviewed changes
@aduh95
lib: do not disable linter for entire files
de4fb11 
Disabling linter for single lines is less error prone.

PR-URL: nodejs#48299
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
@aduh95
aduh95 force-pushed the eslint-disable-lines branch from 975d9ce to de4fb11 Compare June 2, 2023 15:43
@aduh95

aduh95 commented Jun 2, 2023

Copy link
Copy Markdown
Contributor Author

Landed in de4fb11
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit de4fb11 into nodejs:main Jun 2, 2023
@aduh95
aduh95 deleted the eslint-disable-lines branch June 2, 2023 15:44
targos pushed a commit that referenced this pull request Jun 4, 2023
@aduh95 @targos
lib: do not disable linter for entire files
72de4e7 
Disabling linter for single lines is less error prone.

PR-URL: #48299
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
@targos targos mentioned this pull request Jun 4, 2023
v20.3.0 release proposal #48332
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@aduh95 @danielleadams
lib: do not disable linter for entire files
8648373 
Disabling linter for single lines is less error prone.

PR-URL: #48299
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@aduh95 @MoLow
lib: do not disable linter for entire files
286c358 
Disabling linter for single lines is less error prone.

PR-URL: nodejs#48299
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
v18.17.0 release proposal #48694
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@aduh95 @Ceres6
lib: do not disable linter for entire files
4b516f9 
Disabling linter for single lines is less error prone.

PR-URL: nodejs#48299
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@aduh95 @Ceres6
lib: do not disable linter for entire files
f617115 
Disabling linter for single lines is less error prone.

PR-URL: nodejs#48299
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShogunPanda ShogunPanda ShogunPanda approved these changes
@anonrig anonrig anonrig approved these changes
@MoLow MoLow MoLow approved these changes
+2 more reviewers
@targos targos targos approved these changes
@debadree25 debadree25 debadree25 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. fast-track PRs proposed for a shorter-than-standard waiting period before landing. http2 Issues and PRs related to the http2 subsystem. web streams Issues and PRs related to the Web Streams API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@aduh95 @nodejs-github-bot @debadree25 @ShogunPanda @anonrig @targos @MoLow
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.