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.

Sign up for GitHub

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

Jump to bottom

doc: enrich test commands with executable #44347

Merged
merged 10 commits into from
Feb 6, 2023
Merged

doc: enrich test commands with executable #44347

merged 10 commits into from
Feb 6, 2023

Conversation

tony-go
Copy link
Member

@tony-go tony-go commented Aug 22, 2022 •
edited
Loading

Hey 👋

A tiny pull request to enrich test commands in the documentation.

cc @arkerone

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Aug 22, 2022
tony-go
tony-go commented Aug 22, 2022
View reviewed changes
BUILDING.md Outdated Show resolved Hide resolved
@tony-go tony-go marked this pull request as ready for review August 23, 2022 15:16
aduh95
aduh95 reviewed Aug 23, 2022
View reviewed changes
BUILDING.md Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
@tony-go tony-go requested a review from aduh95 August 24, 2022 06:38
aduh95
aduh95 reviewed Aug 24, 2022
View reviewed changes
BUILDING.md Outdated Show resolved Hide resolved
RaisinTen
RaisinTen reviewed Aug 24, 2022
View reviewed changes
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
@tony-go tony-go requested review from aduh95 and RaisinTen and removed request for aduh95 and RaisinTen August 24, 2022 16:53
RaisinTen
RaisinTen approved these changes Aug 25, 2022
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM, would be nice if you could also include the reasoning (#44347 (comment)) in the description

RaisinTen
RaisinTen reviewed Aug 26, 2022
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

After #44407 lands, tools/test.py should work without explicitly writing out the command name. Calling it like python ... would only be required on Windows.

@himself65 himself65 mentioned this pull request Sep 6, 2022
Merged
@tony-go tony-go requested a review from aduh95 September 7, 2022 05:24
@tony-go
Copy link
Member Author

tony-go commented Feb 4, 2023

@RaisinTen I finally updated the doc, could PTAL ^^

@tony-go tony-go requested review from RaisinTen and removed request for aduh95 February 4, 2023 11:36
richardlau
richardlau reviewed Feb 4, 2023
View reviewed changes
BUILDING.md Outdated Show resolved Hide resolved
@tony-go tony-go requested review from richardlau and removed request for RaisinTen February 5, 2023 15:32
@tony-go
Copy link
Member Author

tony-go commented Feb 5, 2023

Finally, remove the python exec from examples and add a note for windows.

RaisinTen
RaisinTen approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2023
@RaisinTen RaisinTen added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 6, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2dacd07 into nodejs:main Feb 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2dacd07

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@tony-go @MylesBorins
doc: enrich test command with executable
a7beac0 
PR-URL: #44347
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
Merged
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@tony-go @danielleadams
doc: enrich test command with executable
f1de3f7 
PR-URL: #44347
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@RaisinTen RaisinTen RaisinTen approved these changes

@richardlau richardlau richardlau approved these changes

@jasnell jasnell jasnell approved these changes

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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tony-go @nodejs-github-bot @jasnell @richardlau @aduh95 @RaisinTen