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

tools: remove readability/fn_size error throw #54744

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

RafaelGSS
Copy link
Member

Follow up #54663

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Sep 3, 2024
@RafaelGSS RafaelGSS added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 3, 2024
Copy link
Contributor

github-actions bot commented Sep 3, 2024

Fast-track has been requested by @RafaelGSS. Please 👍 to approve.

@RedYetiDev RedYetiDev added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 3, 2024
Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

lgtm

@Trott
Copy link
Member

Trott commented Sep 4, 2024

Not a blocker, but a question/observation: This is a patch we'll need to float on top of updates to cpplint.py. Any chance this is worth applying upstream? https://github.com/cpplint/cpplint (Or maybe it doesn't need to be removed but just made more lax?)

@BoscoDomingo
Copy link
Contributor

Anything blocking this PR? It's needed for #53060 🙏🏼

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Sep 5, 2024

Not a blocker, but a question/observation: This is a patch we'll need to float on top of updates to cpplint.py. Any chance this is worth applying upstream? https://github.com/cpplint/cpplint (Or maybe it doesn't need to be removed but just made more lax?)

I can try it later. If I'm reading the code correctly, they ignore the exclude filters and assume fn_size is enabled anyway. We can land this PR while I take a look at it.

@RafaelGSS RafaelGSS added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 5, 2024
@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 Sep 5, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/54744
✔  Done loading data for nodejs/node/pull/54744
----------------------------------- PR info ------------------------------------
Title      tools: remove readability/fn_size error throw (#54744)
Author     Rafael Gonzaga <rafael.nunu@hotmail.com> (@RafaelGSS)
Branch     RafaelGSS:tools-remove-fn-size-lint -> nodejs:main
Labels     c++, tools, fast-track, author ready
Commits    1
 - tools: remove readability/fn_size error throw
Committers 1
 - RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: https://github.com/nodejs/node/pull/54744
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54744
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 03 Sep 2024 19:55:43 GMT
   ✔  Approvals: 1
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/54744#pullrequestreview-2280422138
   ℹ  This PR is being fast-tracked
   ✘  This PR needs to wait 121 more hours to land (or 1 hours if there is one more approval) (or 0 hours if there are 2 more approvals (👍) of the fast-track request from collaborators).
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/10726236123

@RafaelGSS RafaelGSS removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2024
@aduh95 aduh95 removed the fast-track PRs that do not need to wait for 48 hours to land. label Sep 6, 2024
@aduh95
Copy link
Contributor

aduh95 commented Sep 6, 2024

I looks like it's not passing the cpplint CI (cpplint/cpplint#287), I'm not sure we should land it as is.

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 6, 2024
@aduh95
Copy link
Contributor

aduh95 commented Sep 6, 2024

Can you implement cpplint/cpplint@73f2546 instead?

@RafaelGSS
Copy link
Member Author

Can you implement cpplint/cpplint@73f2546 instead?

Sure thing. I'll do it asap

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 7, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 7, 2024
@nodejs-github-bot nodejs-github-bot merged commit 46c6f8c into nodejs:main Sep 7, 2024
22 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 46c6f8c

aduh95 pushed a commit that referenced this pull request Sep 12, 2024
PR-URL: #54744
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
targos pushed a commit that referenced this pull request Sep 22, 2024
PR-URL: #54744
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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
c++ Issues and PRs that require attention from people who are familiar with C++. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants