GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — build: refactor Makefile by RaisinTen · Pull Request #36759 · nodejs/node

Conversation

@RaisinTen

@RaisinTen RaisinTen commented Jan 3, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to Node.js builds or CI infrastructure. label Jan 3, 2021
aduh95
aduh95 reviewed Jan 3, 2021
View reviewed changes
Comment thread Makefile Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Makefile Resolved

Uh oh!

There was an error while loading. Please reload this page.
devsnek
devsnek previously requested changes Jan 3, 2021
View reviewed changes

@devsnek devsnek left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@RaisinTen

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

RaisinTen commented Jan 4, 2021

Copy link
Copy Markdown
Member Author

PTAL 👀
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Jan 4, 2021
View reviewed changes
Comment thread Makefile Resolved

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen
RaisinTen force-pushed the Makefile/replace-echo-with-printf branch from 3733faa to 0149dee Compare January 5, 2021 12:44
@RaisinTen

RaisinTen commented Jan 5, 2021

Copy link
Copy Markdown
Member Author

Does this look okay now?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 approved these changes Jan 5, 2021
View reviewed changes
Comment thread Makefile Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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 with CI started, the required approvals, and no outstanding review comments. label Jan 9, 2021
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 9, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 9, 2021
@nodejs-github-bot

nodejs-github-bot commented Jan 9, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 9, 2021
@jasnell

jasnell commented Jan 9, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

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 :)
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jan 10, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@devsnek
devsnek removed their request for review January 10, 2021 16:17
@devsnek

devsnek commented Jan 10, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 with CI started, the required approvals, and no outstanding review comments. label Jan 10, 2021
@nodejs-github-bot

nodejs-github-bot commented Jan 10, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

aduh95 commented Jan 11, 2021

Copy link
Copy Markdown
Contributor

Landed in 44cf49e
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
v15.6.0 proposal #36889
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
v14.17.0 proposal #38507
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. build Issues and PRs related to Node.js builds or CI infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@RaisinTen @aduh95 @nodejs-github-bot @jasnell @devsnek @richardlau
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.