Skip to content

configure.py: upgrade from optparse to argparse#35755

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
Merged
Trott merged 1 commit into from
Nov 12, 2020
Merged

configure.py: upgrade from optparse to argparse #35755

Trott merged 1 commit into from
Nov 12, 2020

Conversation

@RaisinTen
Copy link
Member

@RaisinTenRaisinTen commented Oct 22, 2020

closes#26725, #29813, #29814

I created this separate repo to test my changes according to this comment:

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?

Originally posted by @cclauss in #29814 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 22, 2020
addaleax
addaleax approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@addaleaxaddaleax left a comment

Choose a reason for hiding this comment

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

/cc @nodejs/python

closes #26725, #29813, #29814

Feel free to add Fixes: lines to the commit message (3 lines, with full URLs to the issues) 🙂

@RaisinTenRaisinTenforce-pushed the upgrade-from-optparse-to-argparse branch from cfb383d to 5251d23CompareOctober 22, 2020 18:11
@codecov-io
Copy link

codecov-io commented Oct 22, 2020

Codecov Report

Merging #35755 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #35755 +/- ## ========================================== - Coverage 87.92% 87.90% -0.02%  ========================================== Files 477 476 -1 Lines 113090 112865 -225 Branches 24632 24598 -34 ========================================== - Hits 99431 99218 -213 - Misses 7948 7955 +7 + Partials 5711 5692 -19
Impacted FilesCoverage Δ
src/connect_wrap.h25.00% <0.00%> (-75.00%)⬇️
lib/internal/source_map/source_map_cache.js84.24% <0.00%> (-5.13%)⬇️
src/node_options.cc85.36% <0.00%> (-1.78%)⬇️
src/inspector_agent.cc83.88% <0.00%> (-0.86%)⬇️
src/node_binding.cc78.94% <0.00%> (-0.48%)⬇️
src/node_worker.cc77.28% <0.00%> (-0.24%)⬇️
lib/internal/util/inspect.js95.53% <0.00%> (-0.15%)⬇️
src/env-inl.h92.78% <0.00%> (-0.06%)⬇️
lib/_http_server.js97.93% <0.00%> (ø)
src/inspector_js_api.cc
... and 4 more

@targos
Copy link
Member

targos commented Oct 23, 2020

@nodejs/python

@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2020
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 23, 2020

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@RaisinTen
Copy link
MemberAuthor

RaisinTen commented Nov 5, 2020

@cclauss hey, here's a gentle reminder in case you forgot to review this PR. 🙂

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@gireeshpunathilgireeshpunathil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 5, 2020
cclauss
cclauss approved these changes Nov 5, 2020
View reviewed changes
Copy link
Contributor

@cclausscclauss left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this!

@RaisinTen
Copy link
MemberAuthor

RaisinTen commented Nov 5, 2020

🙂

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@cclauss
Copy link
Contributor

cclauss commented Nov 9, 2020

@Trott Can you please merge this one. I believe it has all the boxes checked. Thanks.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@gengjiawengengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2020
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 12, 2020

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

@RaisinTen@Trott
build: refactor configure.py to use argparse
f5a86b5
Refs: nodejs#26725Fixes: nodejs#29813 Refs: nodejs#29814 PR-URL: nodejs#35755 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Christian Clauss <[email protected]>
@TrottTrottforce-pushed the upgrade-from-optparse-to-argparse branch from 5251d23 to f5a86b5CompareNovember 12, 2020 19:50
@Trott
Copy link
Member

Trott commented Nov 12, 2020

Landed in f5a86b5

@TrottTrott merged commit f5a86b5 into nodejs:masterNov 12, 2020
@RaisinTenRaisinTen deleted the upgrade-from-optparse-to-argparse branch November 15, 2020 14:29
codebytere pushed a commit that referenced this pull request Nov 22, 2020
@RaisinTen@codebytere
build: refactor configure.py to use argparse
56f83e6
Refs: #26725Fixes: #29813 Refs: #29814 PR-URL: #35755 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Christian Clauss <[email protected]>
@codebyterecodebytere mentioned this pull request Nov 22, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@tniessentniessentniessen approved these changes

@cclausscclausscclauss approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.buildIssues and PRs related to build files or the CI.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@RaisinTen@codecov-io@targos@nodejs-github-bot@cclauss@Trott@addaleax@tniessen@gengjiawen@gireeshpunathil