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.

Sign up for GitHub

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

Jump to bottom

build: apply cpp linting and formatting to ncrypto #55362

Merged
merged 4 commits into from
Nov 10, 2024
Merged

build: apply cpp linting and formatting to ncrypto #55362

merged 4 commits into from
Nov 10, 2024

Conversation

RedYetiDev
Copy link
Member

CC @jasnell (who appears to have contributed most of this work)

If these files are maintained in this repository, should they not follow the same linting / formatting rules? This PR has clang-format and cpplint apply to these files.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels Oct 11, 2024
@RedYetiDev RedYetiDev added the crypto Issues and PRs related to the crypto subsystem. label Oct 11, 2024
@RedYetiDev RedYetiDev marked this pull request as ready for review October 11, 2024 20:00
@RedYetiDev
Copy link
Member Author

CC @nodejs/crypto

@jasnell
Copy link
Member

jasnell commented Oct 11, 2024

Oh nice. Thank you. I've been meaning to get to this. Very helpful

@codecov Codecov
Copy link

codecov bot commented Oct 11, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.91%. Comparing base (68dc15e) to head (786d5fa).
Report is 62 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #55362      +/-   ##
==========================================
- Coverage   88.40%   87.91%   -0.50%     
==========================================
  Files         654      654              
  Lines      187637   187746     +109     
  Branches    36098    35818     -280     
==========================================
- Hits       165887   165057     -830     
- Misses      14994    15885     +891     
- Partials     6756     6804      +48

see 104 files with indirect coverage changes

@jasnell
Copy link
Member

jasnell commented Oct 11, 2024

btw, the intention is to move ncrypto out to a separate repo once all of the work is done to separate it out. It'll be a while tho. At that time that separate project will have its own build and linting stuff

@RedYetiDev RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

Hey, can someone restart the failed builds so this fix can land? Thanks!

@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

@jasnell would you mind re-approving + CI-ing now that a new commit has been added?

aduh95
aduh95 reviewed Nov 5, 2024
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Why would we be linting a vendored dependency? 🤔

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Nov 5, 2024 •
edited
Loading

Why would we be linting a vendored dependency? 🤔

Because we are the author of the dependency. It's our dependency, and it lives in this repository, shouldn't it follow the same lint rules?

If/when ncrypto is moved to its own repository, this can be changed.

@aduh95
Copy link
Contributor

aduh95 commented Nov 5, 2024

the intention is to move ncrypto out to a separate repo once all of the work is done to separate it out. It'll be a while tho.

Does it need to be you doing that, or could it be anyone doing that? It seems to me we should do that instead of applying a linter in deps

@jasnell
Copy link
Member

jasnell commented Nov 6, 2024

Does it need to be you doing that....

No, but the process of separating these out into the dependency is tedious and difficult to get right without introducing breaking changes. I would rather it be done after that work is completed to make code reviews easier. In the meantime the linting and stuff is annoying and applying it here is useful.

aduh95
aduh95 reviewed Nov 6, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
RedYetiDev
RedYetiDev commented Nov 6, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Nov 6, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

mertcanaltin
mertcanaltin approved these changes Nov 9, 2024
View reviewed changes
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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Nov 10, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 10, 2024
@nodejs-github-bot nodejs-github-bot merged commit e7991e8 into nodejs:main Nov 10, 2024
62 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e7991e8

aduh95 pushed a commit that referenced this pull request Nov 16, 2024
@RedYetiDev @aduh95
build: apply cpp linting and formatting to ncrypto
bba7323 
PR-URL: #55362
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 19, 2024
Merged
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@RedYetiDev @tpoisseau
build: apply cpp linting and formatting to ncrypto
6d2408d 
PR-URL: nodejs#55362
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
@RedYetiDev @Ceres6
build: apply cpp linting and formatting to ncrypto
08c3bfd 
PR-URL: nodejs#55362
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mertcanaltin mertcanaltin mertcanaltin approved these changes

@aduh95 aduh95 aduh95 approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. crypto Issues and PRs related to the crypto subsystem. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RedYetiDev @nodejs-github-bot @jasnell @aduh95 @mertcanaltin