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

doc: add note regarding file structure in src/README.md #35000

Closed
wants to merge 1 commit into from

Conversation

lundibundi
Copy link
Member

Refs: #34944 (comment)

Mostly adapted from @addaleax comment (I hope you don't mind) on the files structure since this information seems to be missing from our guides and it would be helpful for people unfamiliar/new with C++.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @nodejs/documentation @Trott

Refs: nodejs#34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>
@lundibundi lundibundi added doc Issues and PRs related to the documentations. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 31, 2020
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 31, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

The text sounds good, but since it’s a stylistic question, maybe rather add it to the style guide?

Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

LGTM

@lundibundi
Copy link
Member Author

@addaleax I'm not sure, I can move it there but personally I'd not expect the info regarding files structure to be in the style guide. Though looking at it right now it already has Do not include *.h if *-inl.h has already been included and other not really stylistic but general advice. So I guess it will be fine to have it there in the Other section? (my main concern is that it won't be as "visible" there compared to this file)

@lundibundi
Copy link
Member Author

ping @addaleax @nodejs/documentation, wdyt #35000 (comment)?

@addaleax
Copy link
Member

@lundibundi I don’t have any strong opinions there, feel free to go with what you think is right :)

@aduh95 aduh95 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 Nov 8, 2020
@aduh95
Copy link
Contributor

aduh95 commented Nov 9, 2020

Landed in da3626a

aduh95 pushed a commit that referenced this pull request Nov 9, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@aduh95 aduh95 closed this Nov 9, 2020
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
Refs: #34944 (comment)
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #35000
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
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. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue Add this label to land a pull request using GitHub Actions. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants