FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: refactor Makefile by RaisinTen · Pull Request #36759 · nodejs/node · GitHub

/ node Public

build: refactor Makefile - #36759

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
RaisinTen:Makefile/replace-echo-with-printf
Jan 11, 2021
ConversationCommits (1)ChecksFiles changed
Merged

build: refactor Makefile#36759
aduh95 merged 1 commit into
nodejs:masterfrom
RaisinTen:Makefile/replace-echo-with-printf

Conversation

RaisinTen commented Jan 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Member
  • add character classes
  • replace echo -n with printf
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 3, 2021
aduh95 reviewed Jan 3, 2021
View reviewed changes
Comment thread Makefile Outdated
Comment thread Makefile Outdated
devsnek previously requested changes Jan 3, 2021
View reviewed changes

devsnek 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This seems to be explicitly worse with the newlines.

RaisinTen commented Jan 4, 2021

Copy link
Copy Markdown
Member Author

In that case, should I replace the printfs that were already there with echos to make things more consistent?

aduh95 commented Jan 4, 2021

Copy link
Copy Markdown
Contributor

In that case, should I replace the printfs that were already there with echos to make things more consistent?

Nah, maybe you can replace echo -n with printf, like ShellCheck recommends.

I'm generally -0 for doing this kind of changes if there's no linter enforcing it, because it's going to drift back to inconsistency eventually. Plus I don't think anyone has ever complained about echo being unsupported on their system.

RaisinTen force-pushed the Makefile/replace-echo-with-printf branch from 20b76b9 to 2e2835d Compare January 4, 2021 15:14
RaisinTen changed the title build: replace echo with printf in Makefile build: refactor Makefile Jan 4, 2021
RaisinTen force-pushed the Makefile/replace-echo-with-printf branch from 2e2835d to 385729b Compare January 4, 2021 15:43

RaisinTen commented Jan 4, 2021

Copy link
Copy Markdown
Member Author

PTAL 👀

aduh95 reviewed Jan 4, 2021
View reviewed changes
Comment thread Makefile Outdated
RaisinTen force-pushed the Makefile/replace-echo-with-printf branch from 3733faa to 0149dee Compare January 5, 2021 12:44

RaisinTen commented Jan 5, 2021

Copy link
Copy Markdown
Member Author

Does this look okay now?

aduh95 approved these changes Jan 5, 2021
View reviewed changes
Comment thread Makefile Outdated
RaisinTen force-pushed the Makefile/replace-echo-with-printf branch from 5763542 to c70d2dd Compare January 5, 2021 15:25
RaisinTen requested a review from devsnek January 5, 2021 15:32
RaisinTen added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 9, 2021
jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 9, 2021
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 9, 2021

nodejs-github-bot commented Jan 9, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35400/

jasnell removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 9, 2021

jasnell commented Jan 9, 2021

Copy link
Copy Markdown
Member

Taking the author ready off given the objection / changes-requested

RaisinTen commented Jan 10, 2021

Copy link
Copy Markdown
Member Author

Taking the author ready off given the objection / changes-requested

@jasnell I had updated the code already according to the request, so the objection is not valid anymore. I couldn't find a Resolve button for this and there is 1 approval already, so I added the author-ready label. Can you please help me in resolving this?

jasnell commented Jan 10, 2021

Copy link
Copy Markdown
Member

Yeah that's fine, just wanted to make sure it was revisited before landing. I'll take a look tomorrow :)

jasnell commented Jan 10, 2021

Copy link
Copy Markdown
Member

@devsnek can you take another look when you get a moment?

devsnek removed their request for review January 10, 2021 16:17

devsnek commented Jan 10, 2021

Copy link
Copy Markdown
Member

not sure how to dismiss in the app but I dismiss my change request

aduh95 dismissed devsnek’s stale review January 10, 2021 17:05

They dismissed their change request in a comment above.

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 10, 2021

nodejs-github-bot commented Jan 10, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35423/

build: refactor Makefile
44cf49e 
* add character classes
* replace echo -n with printf

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#36759
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 force-pushed the Makefile/replace-echo-with-printf branch from c70d2dd to 44cf49e Compare January 11, 2021 10:37

aduh95 commented Jan 11, 2021

Copy link
Copy Markdown
Contributor

Landed in 44cf49e

aduh95 merged commit 44cf49e into nodejs:master Jan 11, 2021
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
build: refactor Makefile
21f3295 
* add character classes
* replace echo -n with printf

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #36759
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RaisinTen deleted the Makefile/replace-echo-with-printf branch January 12, 2021 13:28
danielleadams mentioned this pull request Jan 12, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
build: refactor Makefile
5c74dc7 
* add character classes
* replace echo -n with printf

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #36759
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams mentioned this pull request May 3, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

richardlau richardlau left review comments
aduh95 aduh95 approved these changes
+1 more reviewer
devsnek devsnek left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. build Issues and PRs related to build files or the CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL