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

build: apply cpp linting and formatting to ncrypto #55362

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.91%. Comparing base (68dc15e) to head (eac6d1d).
Report is 22 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   187655      +18     
  Branches    36098    35803     -295     
==========================================
- Hits       165887   164977     -910     
- Misses      14994    15877     +883     
- Partials     6756     6801      +45     

see 100 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.

@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev
Copy link
Member Author

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

@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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
Development

Successfully merging this pull request may close these issues.

3 participants