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

remove default values from argparse #2453

Merged
merged 16 commits into from
Jun 3, 2024
Merged

Conversation

ovatman
Copy link
Contributor

@ovatman ovatman commented May 30, 2024

Since we are handling default values for command line options in the specific options classes a default value shouldn't be assigned during cli argument parsing when option is absent. This PR removes default values from cli argument parser.

@ovatman ovatman self-assigned this May 30, 2024
@ovatman ovatman requested a review from nwatson22 May 30, 2024 11:11
@ovatman ovatman added the bug Something isn't working label May 30, 2024
@ovatman
Copy link
Contributor Author

ovatman commented May 31, 2024

A final note:
Even though argparse's documentation says the default parameter itself defaults to None (see here), it defaults to False for bool parameters, causing the last commit here.

@ovatman ovatman marked this pull request as ready for review May 31, 2024 17:15
@ovatman ovatman requested a review from anvacaru June 3, 2024 06:06
@rv-jenkins rv-jenkins merged commit efeb85f into master Jun 3, 2024
11 checks passed
@rv-jenkins rv-jenkins deleted the tolga/argparse-remove-defaults branch June 3, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants