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

configure.py: upgrade from optparse to argparse #35755

Merged
merged 1 commit into from
Nov 12, 2020
Merged

configure.py: upgrade from optparse to argparse #35755

merged 1 commit into from
Nov 12, 2020

Conversation

RaisinTen
Copy link
Contributor

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-bot nodejs-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

@addaleax addaleax 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) 🙂

@RaisinTen RaisinTen force-pushed the upgrade-from-optparse-to-argparse branch from cfb383d to 5251d23 Compare October 22, 2020 18:11
@codecov-io
Copy link

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 Files Coverage Δ
src/connect_wrap.h 25.00% <0.00%> (-75.00%) ⬇️
lib/internal/source_map/source_map_cache.js 84.24% <0.00%> (-5.13%) ⬇️
src/node_options.cc 85.36% <0.00%> (-1.78%) ⬇️
src/inspector_agent.cc 83.88% <0.00%> (-0.86%) ⬇️
src/node_binding.cc 78.94% <0.00%> (-0.48%) ⬇️
src/node_worker.cc 77.28% <0.00%> (-0.24%) ⬇️
lib/internal/util/inspect.js 95.53% <0.00%> (-0.15%) ⬇️
src/env-inl.h 92.78% <0.00%> (-0.06%) ⬇️
lib/_http_server.js 97.93% <0.00%> (ø)
src/inspector_js_api.cc
... and 4 more

@targos
Copy link
Member

targos commented Oct 23, 2020

@nodejs/python

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2020
@github-actions github-actions bot 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

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
Contributor Author

@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
@gireeshpunathil gireeshpunathil 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

@cclauss cclauss 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
Contributor Author

🙂

@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
@gengjiawen gengjiawen added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2020
@github-actions github-actions bot 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

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

@RaisinTen @Trott
build: refactor configure.py to use argparse
f5a86b5 
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>
@Trott Trott force-pushed the upgrade-from-optparse-to-argparse branch from 5251d23 to f5a86b5 Compare November 12, 2020 19:50
@Trott
Copy link
Member

Trott commented Nov 12, 2020

Landed in f5a86b5

@Trott Trott merged commit f5a86b5 into nodejs:master Nov 12, 2020
@RaisinTen RaisinTen 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: #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>
@codebytere codebytere mentioned this pull request Nov 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cclauss cclauss cclauss approved these changes

@tniessen tniessen tniessen approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott 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.
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