Skip to content

build: refactor Makefile#36759

Merged
aduh95 merged 1 commit intonodejs:masterfrom
RaisinTen:Makefile/replace-echo-with-printf
Jan 11, 2021
Merged

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

Conversation

@RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented Jan 3, 2021 •
edited
Loading

  • 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 nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 3, 2021
aduh95
aduh95 reviewed Jan 3, 2021
View reviewed changes
devsnek
devsnek previously requested changes Jan 3, 2021
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

This seems to be explicitly worse with the newlines.

@RaisinTen
Copy link
Member Author

RaisinTen commented Jan 4, 2021

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

@aduh95
Copy link
Contributor

aduh95 commented Jan 4, 2021

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 RaisinTen force-pushed the Makefile/replace-echo-with-printf branch from 20b76b9 to 2e2835d Compare January 4, 2021 15:14
@RaisinTen RaisinTen changed the title build: replace echo with printf in Makefile build: refactor Makefile Jan 4, 2021
@RaisinTen RaisinTen force-pushed the Makefile/replace-echo-with-printf branch from 2e2835d to 385729b Compare January 4, 2021 15:43
@RaisinTen
Copy link
Member Author

RaisinTen commented Jan 4, 2021

PTAL 👀

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

RaisinTen commented Jan 5, 2021

Does this look okay now?

aduh95
aduh95 approved these changes Jan 5, 2021
View reviewed changes
@RaisinTen RaisinTen force-pushed the Makefile/replace-echo-with-printf branch from 5763542 to c70d2dd Compare January 5, 2021 15:25
@RaisinTen RaisinTen requested a review from devsnek January 5, 2021 15:32
@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 9, 2021
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 9, 2021
@github-actions 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
Copy link
Collaborator

nodejs-github-bot commented Jan 9, 2021

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

@jasnell jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 9, 2021
@jasnell
Copy link
Member

jasnell commented Jan 9, 2021

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

@RaisinTen
Copy link
Member Author

RaisinTen commented Jan 10, 2021

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
Copy link
Member

jasnell commented Jan 10, 2021

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

@jasnell
Copy link
Member

jasnell commented Jan 10, 2021

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

@devsnek devsnek removed their request for review January 10, 2021 16:17
@devsnek
Copy link
Member

devsnek commented Jan 10, 2021

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

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

They dismissed their change request in a comment above.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 10, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 10, 2021

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

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

Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: nodejs#36759
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95 aduh95 force-pushed the Makefile/replace-echo-with-printf branch from c70d2dd to 44cf49e Compare January 11, 2021 10:37
@aduh95
Copy link
Contributor

aduh95 commented Jan 11, 2021

Landed in 44cf49e

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

Co-authored-by: Antoine du Hamel <[email protected]>

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

Co-authored-by: Antoine du Hamel <[email protected]>

PR-URL: #36759
Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardlau richardlau richardlau left review comments

@aduh95 aduh95 aduh95 approved these changes

+1 more reviewer

@devsnek 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 pending requests for changes, 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

@RaisinTen @aduh95 @nodejs-github-bot @jasnell @devsnek @richardlau