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

node-api: format Node-API related code #42396

Closed
wants to merge 1 commit into from

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Mar 18, 2022

Format Node-API related source files using Clang Format.

There are no code changes besides the code formatting.

When we work on changes to Node-API it is very difficult to write code that satisfies the lint-cpp requirements because most of code is not formatted correctly. So, we often need to write new code, format file, copy new code, undo formatting, and then paste the new code. Instead, we must simplify the development process by allowing the code formatting that uses Clang Format and the .clang-format specification in the root of the Node.JS project. This PR updates formatting of Node-API related files so that in future developers could simply format the whole file after implementing a change.
Ideally, the lint-cpp must verify that all changed files are formatted correctly per .clang-format specification.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 18, 2022
bool escape_called() const {
return escape_called_;
}
bool escape_called() const { return escape_called_; }
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a great opportunity to also align the Node-API code with our C++ style guide – it varies in quite a few aspects, esp. around name casing/underscore placement/etc.. It’s usually not important enough to make a fuss about, but since this PR is already about style changes anyway … 🙂

Copy link
Member Author

@vmoroz vmoroz Mar 20, 2022

Choose a reason for hiding this comment

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

Good point! Let me do it in a follow up PR. I would like to keep this PR focusing only on the automated formatting without any other code changes.
One of my biggest concerns with this PR is how important the order of includes.
It seems that all tests are passing, and I assume that all should be fine.
Thank you for the sign off!

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2022
@nodejs-github-bot
Copy link
Collaborator

mhdawson pushed a commit that referenced this pull request Mar 21, 2022
PR-URL: #42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@mhdawson
Copy link
Member

Landed in 718be08

@mhdawson mhdawson closed this Mar 21, 2022
@vmoroz
Copy link
Member Author

vmoroz commented Mar 22, 2022

Thank you @mhdawson for landing it!

juanarbol pushed a commit that referenced this pull request Apr 4, 2022
PR-URL: #42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
PR-URL: nodejs#42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
This was referenced Apr 5, 2022
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
PR-URL: #42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
PR-URL: nodejs#42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
PR-URL: #42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
PR-URL: #42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#42396
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Darshan Sen <raisinten@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++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants