The Wayback Machine - https://web.archive.org/web/20201023141317/https://github.com/nodejs/node/pull/35755
Skip to content

/ node

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

Jump to bottom

configure.py: upgrade from optparse to argparse #35755

Open
wants to merge 1 commit into
base: master
from
Open

configure.py: upgrade from optparse to argparse #35755

wants to merge 1 commit into from
+202 −148

Conversation

@RaisinTen
Copy link

@RaisinTen RaisinTen 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
@addaleax
addaleax approved these changes Oct 22, 2020
View changes
Copy link
Member

@addaleax addaleax left a comment

/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
refactor: upgrade from optparse to argparse
Loading status checks…
5251d23 
Fixes:
#26725
#29813
#29814
@RaisinTen RaisinTen force-pushed the RaisinTen:upgrade-from-optparse-to-argparse branch from cfb383d to 5251d23 Oct 22, 2020
@codecov-io
Copy link

@codecov-io 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 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 targos commented Oct 23, 2020

@nodejs/python
@targos targos added the request-ci label Oct 23, 2020
@github-actions github-actions bot removed the request-ci label Oct 23, 2020
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Oct 23, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/33812/
@gengjiawen gengjiawen requested a review from cclauss Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax

@cclauss cclauss

Assignees
No one assigned
Labels
build
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@RaisinTen @codecov-io @targos @nodejs-github-bot @addaleax
You can’t perform that action at this time.