Skip to content

Add a header in BUILDING.md - #35710

Merged
Trott merged 1 commit into
nodejs:masterfrom
PoojaDurgad:build-install
Oct 22, 2020
Merged

Add a header in BUILDING.md#35710
Trott merged 1 commit into
nodejs:masterfrom
PoojaDurgad:build-install

Conversation

@PoojaDurgad

Copy link
Copy Markdown
Contributor
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

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Oct 19, 2020
Comment threadBUILDING.md Outdated
@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #35710 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #35710 +/- ##
==========================================
+ Coverage 96.40% 96.41% +0.01% 
==========================================
Files 220 220 Lines 73675 73675 ==========================================
+ Hits 71025 71033 +8 + Misses 2650 2642 -8 
Impacted FilesCoverage Δ
lib/internal/modules/cjs/loader.js95.03% <0.00%> (+0.64%)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b18651b...21709aa. Read the comment docs.

@TrottTrott left a comment

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.

The first commit message needs to be more descriptive. The subsequent commit message should be a "fixup" commit message or else squashed into the first. Otherwise, the bot will land it as a separate commit.

The content is might not be in the best place. It separates the macos firewall tool from Running Tests and those two things should go together. I think it's fine (better even?) for the install instructions to be located where they were before.

@Trott

Copy link
Copy Markdown
Member

The first commit message needs to be more descriptive. The subsequent commit message should be a "fixup" commit message or else squashed into the first. Otherwise, the bot will land it as a separate commit.

Since the commit message needed only a small tweak, I rebased, squashed, and force-pushed. I hope I didn't mess up your workflow, and if I did, I apologize in advance!

@Trott

Copy link
Copy Markdown
Member

The content is might not be in the best place. It separates the macos firewall tool from Running Tests and those two things should go together. I think it's fine (better even?) for the install instructions to be located where they were before.

Having second thoughts about my comment here. Maybe it's fine where it is. And if not, we can move it some other time. Sorry to leave a wall of comments and then walk back half of them.

@PoojaDurgad

Copy link
Copy Markdown
ContributorAuthor

@Trott - no problem. thanks

PR-URL: nodejs#35710
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott

Copy link
Copy Markdown
Member

Landed in 1ba932b

@Trott
Trott merged commit 1ba932b into nodejs:masterOct 22, 2020
targos pushed a commit that referenced this pull request Nov 3, 2020
PR-URL: #35710
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targostargos mentioned this pull request Nov 3, 2020
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
PR-URL: #35710
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #35710
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #35710
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.docIssues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@PoojaDurgad@codecov-io@Trott@richardlau@Jurgis586@nodejs-github-bot