Skip to content

meta: add N-API to codeowners coverage #34039

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

Closed
mhdawson wants to merge 4 commits into from
Closed

meta: add N-API to codeowners coverage #34039

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d66b000
meta: add N-API to codeowners coverage
mhdawson Jun 24, 2020
30897f3
squash: address comments
mhdawson Jun 24, 2020
0ab5c01
Update .github/CODEOWNERS
mhdawson Jul 2, 2020
c6e2806
accept comments
mhdawson Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update .github/CODEOWNERS 
Co-authored-by: Richard Lau <riclau@uk.ibm.com>
  • Loading branch information
@mhdawson @richardlau
mhdawson and richardlau authored Jul 2, 2020
commit 0ab5c019dc21c0afea86f8d8bbc62817c7172e3c
4 changes: 2 additions & 2 deletions .github/CODEOWNERS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,5 @@

# ./src/node_api* @nodejs/n-api

@devsnek devsnek Jun 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

are these supposed to be commented out

@mhdawson mhdawson Jun 24, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@devsnek that is a good question. I saw most of them were commented out so followed the same pattern. @jasnell since you left many of the them commented out for now is it ok to turn it on for N-API now or do you think we should let the initial set bake for a while? Either way I'd like to add N-API now (either active or commented out) so that we don't forget.

@mmarchini mmarchini Jun 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The ones commented out are for teams which does not have write permission on this repository (teams which are not subteams of collaborators), as per discussions in #33895.

I'm working on an alternative to codeowners using the github-bot to work around GitHub's constraint on only teams with write permissions being valid owners. IMO it's best to keep these commented out for now, then we can uncomment when we move to the github-bot-based owners. Note that this PR is essentially a no-op this way though.

# ./src/js_native_api* @nodejs/n-api

@richardlau richardlau Jun 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the relevant doc pages also require sign off?

@mhdawson mhdawson Jun 24, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@richardlau makes sense added.

# ./doc/guides/adding-new-napi-api.md
# ./doc/api/n-api.md
# ./doc/guides/adding-new-napi-api.md @nodejs/n-api
# ./doc/api/n-api.md @nodejs/n-api