build: refactor configure.py by VoltrexKeyva · Pull Request #47323 · nodejs/node

VoltrexKeyva · 2023-03-30T17:14:56Z

Explicitly specify the encoding when opening files.
Use f-strings to format strings.
Use isinstance() for type checks instead of type().
Use the with keyword for resource-allocating operations.
Avoid using multiple statements in a single line.
Remove unnecessary else clauses after return.
Iterate with the items() method of dictionaries when both the key and value are used.
Remove unnecessary parentheses.
Rename unused unpacked variables to _, _1, etc etc.
Rename the list variable to avoid conflict with the global list() function.
Remove unused path parameter of the icu_download() function.
Use the pathlib library for paths instead of os.path.

VoltrexKeyva · 2023-03-30T17:25:25Z

Note that the tests are failing because the packaging library is not installed, I'm not sure where to add the command to install it so that it applies to all the tests that run configure.py.

richardlau · 2023-03-30T18:27:31Z

Replace usage of the deprecated distutils library with the packaging library recommended by setuptools' "Porting from Distutils" guide.

FWIW some previous discussion in #42186. packaging not being part of the Python stdlib is going to be a pain for everyone/new contributors.

VoltrexKeyva · 2023-03-31T18:09:35Z

packaging not being part of the Python stdlib is going to be a pain for everyone/new contributors.

I'm not sure about that, we'll have to switch either way though we could make our own class/function implementing version parsing but it's too much work for the simple use cases here, so I'd say we should make the move to the packaging library, and of course, add it to the build instructions if automatic installation is not a preferable choice.

VoltrexKeyva · 2023-04-05T14:34:39Z

I'm not sure about this change, the path parameter of the icu_download() function was never used (not inside the function I mean), not even from the day it was added.
But an argument to the path parameter is being passed here:
node/configure.py
Line 1819 in 069365c
localzip=icu_download(icu_full_path)
even though it's not used inside the icu_download() function, is this a mistake or was it supposed to be used?

👍 Let's revert this change and put a # noqa here to placate the linter.

Why should we revert this? Do you want me to make this change in a separate PR?

targos · 2023-04-12T08:19:27Z

@nodejs/python

cclauss

This is a legacy codebase that a lot of folks depend on so we want to keep any possible breakage to a minimum. Here are some ideas...

Also, I would like to land #47519 before we land this so we have some stronger guardrails in place.

cclauss · 2023-04-12T08:42:39Z

👍 Let's revert this change and put a # noqa here to placate the linter.

cclauss

more ideas

I know we are only changing a single file but it is a vital file and I do not know if it has enough tests... Perhaps we should introduce these changes in multiple PRs. Your thoughts on test coverage?

- Explicitly specify the encoding when opening files. - Use f-strings to format strings. - Use `isinstance()` for type checks instead of `type()`. - Use the `with` keyword for resource-allocating operations. - Avoid using multiple statements in a single line. - Remove unnecessary `else` clauses after `return`. - Iterate with the `items()` method of dictionaries when both the key and value are used. - Remove unnecessary parentheses. - Rename unused unpacked variables to `_`, `_1`, etc etc. - Rename the `list` variable to avoid conflict with the global `list()` function. - Remove unused `path` parameter of the `icu_download()` function. - Use the `pathlib` library for paths instead of `os.path`.

VoltrexKeyva · 2023-04-16T11:49:43Z

Perhaps we should introduce these changes in multiple PRs.

I'm fine with that, should I introduce each of these changes in separate PRs to reduce review time?

Your thoughts on test coverage?

Writing new tests for such build files sounds like a good idea, but I have no experience writing tests for such files 😅.

cclauss · 2023-04-16T11:48:28Z

 valid_intl_modes = ('none', 'small-icu', 'full-icu', 'system-icu')
-with open ('tools/icu/icu_versions.json') as f:
- icu_versions = json.load(f)
+icu_versions = json.loads((tools_path / 'icu' / 'icu_versions.json').read_text())


.read_text(encoding='utf-8') is also possible.

cclauss · 2023-04-16T11:57:02Z

I think we should get this PR will all changes ready and with all tests passing. Then we should have a second PR that is a strict subset of these changes. We land the second PR first and then after a release and user feedback, we rebase this PR and push the remaining into the next Node release. All the pathlib changes should land separately from all other changes. Thoughts? Or am I just being too paranoid?

VoltrexKeyva · 2023-04-16T11:59:42Z

Then we should have a second PR that is a strict subset of these changes.

What changes here counts as a strict subset?

All the pathlib changes should land separately from all other changes. Thoughts? Or am I just being too paranoid?

That's a good idea.

cclauss · 2023-04-16T12:18:21Z

What changes here counts as a strict subset?

The pathlib changes vs. all non-pathlib changes.

VoltrexKeyva · 2023-04-22T08:50:20Z

Closing this since there are now two different PRs, one for pathlib changes (#47581) and one for non-pathlib changes (#47667).

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment)nodejs#47323 (comment)

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: #47323 (comment)#47323 (comment) PR-URL: #47581 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Christian Clauss <cclauss@me.com>

Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment)nodejs#47323 (comment) PR-URL: nodejs#47581 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Christian Clauss <cclauss@me.com>

srl295 · 2024-10-01T14:41:14Z

broke ICU downloading on windows (the 'path' argument is a URL or a file path) - see #55214

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Mar 30, 2023

VoltrexKeyva force-pushed the refact-conf branch 2 times, most recently from 762c432 to 14b08f7Compare March 30, 2023 17:32

Trott added the python PRs and issues that require attention from people who are familiar with Python. label Mar 30, 2023

VoltrexKeyva force-pushed the refact-conf branch from 14b08f7 to 4863d58Compare April 4, 2023 15:15

targos reviewed Apr 5, 2023
View reviewed changes

Comment threadconfigure.py Outdated

VoltrexKeyva force-pushed the refact-conf branch 3 times, most recently from f2d5605 to e6931deCompare April 5, 2023 14:23

VoltrexKeyva commented Apr 5, 2023
View reviewed changes

VoltrexKeyva force-pushed the refact-conf branch from e6931de to 9a0a399Compare April 10, 2023 08:39

VoltrexKeyva requested review from richardlau and targos April 10, 2023 22:21

targos removed their request for review April 12, 2023 08:19

cclauss reviewed Apr 12, 2023
View reviewed changes

Comment threadconfigure.py Outdated

cclauss requested changes Apr 12, 2023
View reviewed changes

VoltrexKeyva force-pushed the refact-conf branch 2 times, most recently from ed5706f to 89e91edCompare April 15, 2023 10:07

This comment was marked as outdated.
Sign in to view

VoltrexKeyva force-pushed the refact-conf branch 3 times, most recently from edf05e6 to ff0ff9bCompare April 16, 2023 10:08

cclauss requested changes Apr 16, 2023
View reviewed changes

Comment threadconfigure.py Outdated
Comment threadconfigure.py Outdated
Comment threadconfigure.py Outdated
Comment threadconfigure.py Outdated
Comment threadconfigure.py Outdated
Comment threadconfigure.py Outdated

VoltrexKeyva force-pushed the refact-conf branch 2 times, most recently from dec8698 to f3c8e7fCompare April 16, 2023 11:23

VoltrexKeyva force-pushed the refact-conf branch from f3c8e7f to 68ef467Compare April 16, 2023 11:46

cclauss reviewed Apr 16, 2023
View reviewed changes

VoltrexKeyva mentioned this pull request Apr 16, 2023
build: use pathlib for paths #47581
Merged

VoltrexKeyva closed this Apr 22, 2023

VoltrexKeyva deleted the refact-conf branch April 22, 2023 08:50

VoltrexKeyva added a commit to VoltrexKeyva/node that referenced this pull request May 2, 2023
build: use pathlib for paths
ec01f70
Use Python's `pathlib` library for paths and related operations instead of `os.path`. Refs: nodejs#47323 (comment)nodejs#47323 (comment)

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Oct 1, 2024

Uh oh!

Uh oh!

Conversation

VoltrexKeyva commented Mar 30, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VoltrexKeyva commented Mar 30, 2023

Uh oh!

richardlau commented Mar 30, 2023

Uh oh!

VoltrexKeyva commented Mar 31, 2023

Uh oh!

Uh oh!

VoltrexKeyvaApr 5, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cclaussApr 12, 2023

Choose a reason for hiding this comment

Uh oh!

VoltrexKeyvaApr 16, 2023

Choose a reason for hiding this comment

Uh oh!

targos commented Apr 12, 2023

Uh oh!

Uh oh!

cclauss left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cclaussApr 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

cclauss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VoltrexKeyva commented Apr 16, 2023

Uh oh!

cclaussApr 16, 2023

Choose a reason for hiding this comment

Uh oh!

cclauss commented Apr 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VoltrexKeyva commented Apr 16, 2023

Uh oh!

cclauss commented Apr 16, 2023

Uh oh!

VoltrexKeyva commented Apr 22, 2023

Uh oh!

srl295 commented Oct 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

VoltrexKeyva commented Mar 30, 2023•
edited
Loading

VoltrexKeyvaApr 5, 2023•
edited
Loading

cclauss left a comment •
edited
Loading

cclauss commented Apr 16, 2023•
edited
Loading