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.

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

build: replace optparse for argparse in configure.py #29814

Closed
wants to merge 2 commits into from
Closed

build: replace optparse for argparse in configure.py #29814

wants to merge 2 commits into from

Conversation

vccolombo
Copy link

Hello! I saw Issue #29813 and decided to take the challenge as my first commit to Node.
However while looking on what was done in PR #26725 I got confused about what is gyp_args and what that PR changed about it.

Currently, my PR fails when running gyp_args += args with error TypeError: 'Namespace' object is not iterable

Can anyone give me some help about this?

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • 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 Oct 2, 2019
configure.py Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
configure.py Outdated Show resolved Hide resolved
Issue #29813 suggested migrating from optparse to argparse as optparse is deprecated.
This commit does the changes necessary to this migration, as suggested in https://docs.python.org/3/library/argparse.html#module-argparse
Most of the work was done in PR #26725
Fixes: #29813
@richardlau
Copy link
Member

However while looking on what was done in PR #26725 I got confused about what is gyp_args and what that PR changed about it.

Currently, my PR fails when running gyp_args += args with error TypeError: 'Namespace' object is not iterable

Probably needs someone more familiar with Python (cc @nodejs/python) but with optparse the parse() method returns args that were not consumed by the parser. I think this is an array (e.g. like sys.argv). A quick glance at the docs for argparse suggests the equivalent may be to use parse_known_args().

@cclauss cclauss added Hacktoberfest python PRs and issues that require attention from people who are familiar with Python. labels Oct 2, 2019
@cclauss
Copy link
Contributor

cclauss commented Oct 2, 2019

My method of approaching this would be a separate repo to prove it all out. I would have one file that would be all the optparse stuff with as minimal changes as possible. I would have a second file that was my proposed solution using argparse. Each file would take in commandline args and print out the parsed args. Then I would have a test runner (Travis CI, GitHub Actions, CircleCI, etc.) and I would run the same set of inputs through both and see if the outputs matched. Does that make sense?

@cclauss
Copy link
Contributor

cclauss commented Oct 2, 2019

As discussed at https://docs.python.org/2/library/argparse.html#argparse.Namespace try vars(args) to get a sense of what is inside.

@vccolombo
Copy link
Author

Thank you very much for your help. I need some time to try it and I'll come back after that

@vccolombo
Copy link
Author

Hello there. Really sorry for the delay. I got stuck with some stuff from college. I may need a couple of weeks to deliver something here. But I still want to work on and finish this. Hope you understand

Thanks!

@cclauss
Copy link
Contributor

cclauss commented Oct 15, 2019

No rush on this one given that both Python 3.8 (released yesterday) and 3.9 (1+ years away) support the deprecated module. If you are not warped up in one month then I will take a whack at it.

@BridgeAR
Copy link
Member

Ping @vccolombo @cclauss

@vccolombo
Copy link
Author

Hi!

I wasn't able to finish it, sorry about that. I though it would be easier when I saw the Issue, and now I'm on a new job so it's even harder to find time to tackle it. Really sorry again.

Thank you for your time

@vccolombo vccolombo closed this Jan 12, 2020
@BridgeAR
Copy link
Member

@vccolombo thank you for the work!

Trott pushed a commit to RaisinTen/node that referenced this pull request Nov 12, 2020
Refs: nodejs#26725
Fixes: nodejs#29813
Refs: nodejs#29814

PR-URL: nodejs#35755
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
codebytere pushed a commit that referenced this pull request Nov 22, 2020
Refs: #26725
Fixes: #29813
Refs: #29814

PR-URL: #35755
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants