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: add trailing commas in event tests #45466

Merged
merged 1 commit into from Nov 22, 2022
Merged

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 14, 2022

As much as I would like to do this everywhere and then modify the lint rule to enforce it, the churn would be too big. However if we're going to have relatively frequent nits for this sort of thing (as we do), I'd prefer we migrate a few files at a time to never actually getting around to doing it.

Ref: #45448 (review)

@Trott Trott requested a review from aduh95 November 14, 2022 22:47
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 14, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: nodejs#45448 (review)
@aduh95
Copy link
Contributor

aduh95 commented Nov 15, 2022

I'm not sure it's worth merging this if it's not backup by a lint rule, it's going to create conflicts on other PRs and "pollute" the git history with no assurance that it's not going to be reverted. The upside of using trailing commas is to minimize the git diff, making git blame more helpful, I think adding commits that add trailing commas defeats that purpose.
I've opened #45468 to try to have a lint rule.

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 21, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 21, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/45466
✔  Done loading data for nodejs/node/pull/45466
----------------------------------- PR info ------------------------------------
Title      test: add trailing commas in event tests (#45466)
Author     Rich Trott  (@Trott)
Branch     Trott:event -> nodejs:main
Labels     test, needs-ci
Commits    1
 - test: add trailing commas in event tests
Committers 1
 - Rich Trott 
PR-URL: https://github.com/nodejs/node/pull/45466
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45466
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 14 Nov 2022 22:47:00 GMT
   ✔  Approvals: 1
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/45466#pullrequestreview-1183484182
   ✔  Last GitHub CI successful
   ✖  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3518901769

@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 22, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 22, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 22, 2022
@nodejs-github-bot nodejs-github-bot merged commit d5d7a41 into nodejs:main Nov 22, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in d5d7a41

@Trott Trott deleted the event branch November 22, 2022 23:19
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: nodejs#45448 (review)
PR-URL: nodejs#45466
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Nov 24, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
As much as I would like to do this everywhere and then modify the lint
rule to enforce it, the churn would be too big. However if we're going
to have relatively frequent nits for this sort of thing (as we do), I'd
prefer we migrate a few files at a time to never actually getting around
to doing it.

Ref: #45448 (review)
PR-URL: #45466
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants