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

tools: refactor checkimports.py #50011

Merged
merged 1 commit into from
Oct 15, 2023
Merged

tools: refactor checkimports.py #50011

merged 1 commit into from
Oct 15, 2023

Conversation

VoltrexKeyva
Copy link
Member

@VoltrexKeyva VoltrexKeyva commented Oct 2, 2023 •
edited
Loading

  • Use f-strings for formatting.
  • Use raw strings for regexes alongside f-strings.
  • Use a generator.
  • Remove unnecessary else clause.

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 2, 2023
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 2, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 2, 2023
@VoltrexKeyva
Copy link
Member Author

@nodejs/python need one more approval to land.

@cclauss
Copy link
Contributor

cclauss commented Oct 4, 2023 •
edited
Loading

I am not a fan of the shebang change. We should join the broad effort to encourage users to use https://docs.python.org/3/library/venv.html to avoid creating dependency conflicts across projects. In a venv, python is defined and points to the same Python version that pip is using to install that project's dependencies.

Today I did brew install python@3.12 and now I can no longer do python3.12 -m pip install --upgrade pip because I get an error: externally-managed-environment which is probably something that many of you Linux users have gotten used to seeing.

The Python core team has worked with OS vendors to lock down the system Python and encourage the use of venv which defines a safe python that is isolated for the dependencies of this project and not all projects on the machine.

@VoltrexKeyva
Copy link
Member Author

@cclauss PTAL.

@VoltrexKeyva VoltrexKeyva added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Oct 10, 2023
Open
24 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Oct 11, 2023
Open
30 tasks
@VoltrexKeyva
tools: refactor checkimports.py
a036e93 
- Use f-strings for formatting.
- Use raw strings for regexes alongside f-strings.
- Use a generator.
- Remove unnecessary `else` clause.
@VoltrexKeyva VoltrexKeyva added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@VoltrexKeyva
Copy link
Member Author

@cclauss will need a re-approval to land.

@cclauss
Copy link
Contributor

cclauss commented Oct 15, 2023 •
edited
Loading

See Merging is blocked below.

I do not know how to translate that into English.

@VoltrexKeyva
Copy link
Member Author

See Merging is blocked below.

I do not know how to translate that into English.

That's intentional, refer to the second point in the collaborator guide's landing pull requests section.

@VoltrexKeyva VoltrexKeyva added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2023
@nodejs-github-bot nodejs-github-bot merged commit 31bde06 into nodejs:main Oct 15, 2023
53 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 31bde06

@VoltrexKeyva VoltrexKeyva deleted the refact-py1 branch October 15, 2023 15:16
kumarrishav pushed a commit to kumarrishav/node that referenced this pull request Oct 16, 2023
@VoltrexKeyva @kumarrishav
tools: refactor checkimports.py
f79cbef 
- Use f-strings for formatting.
- Use raw strings for regexes alongside f-strings.
- Use a generator.
- Remove unnecessary `else` clause.

PR-URL: nodejs#50011
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
targos pushed a commit that referenced this pull request Oct 23, 2023
@VoltrexKeyva @targos
tools: refactor checkimports.py
bf7b94f 
- Use f-strings for formatting.
- Use raw strings for regexes alongside f-strings.
- Use a generator.
- Remove unnecessary `else` clause.

PR-URL: #50011
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
@targos targos mentioned this pull request Oct 23, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@VoltrexKeyva @alexfernandez
tools: refactor checkimports.py
7149c88 
- Use f-strings for formatting.
- Use raw strings for regexes alongside f-strings.
- Use a generator.
- Remove unnecessary `else` clause.

PR-URL: nodejs#50011
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
@VoltrexKeyva @targos
tools: refactor checkimports.py
35b160e 
- Use f-strings for formatting.
- Use raw strings for regexes alongside f-strings.
- Use a generator.
- Remove unnecessary `else` clause.

PR-URL: #50011
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
@targos targos mentioned this pull request Nov 12, 2023
Merged
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@cclauss cclauss cclauss 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. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VoltrexKeyva @nodejs-github-bot @cclauss @richardlau