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

Make CLI in line with intended default for increment_micro #329

Conversation

znichollscr
Copy link
Contributor

@carstencodes I think this would fix #326,
but I'm not certain about any side effects.

A different solution would be to use store_const
and have different None handling, see e.g.
https://docs.python.org/3/library/argparse.html.
Again, not sure of side effects.

@znichollscr
Copy link
Contributor Author

Hmm the fact that the tests still pass also suggest there's a missing test case somewhere..

@carstencodes
Copy link
Owner

Yeah, tests are not fully covered yet.

Currently, changing the command-line argument would introduce a breaking change.

Sorry for being a little cautious about this, but I might find a workaround using meta-vars...

Give me a day or two

@znichollscr
Copy link
Contributor Author

No problem, take your time

@znichollscr
Copy link
Contributor Author

As far as I could tell, the CLI is setting the increment_micro option so the defaults deeper within the codebase are just overwritten

@carstencodes
Copy link
Owner

Ah, as the smallest supported python version is 3.9, the argparse.BooleanOptionalAction is available (cf manual):

import argparse

parser = argparse.ArgumentParser()
parser.add_argument('--foo', action=argparse.BooleanOptionalAction)  # Added in version 3.9.
parser.parse_args(['--no-foo'])
Namespace(foo=False)

This should be combinable with default values.

So, a possible solution would be

sub_parser.add_argument(
            "--micro",
            action=argparse.BooleanOptionalAction,
            dest="increment_micro",
            help="When setting pre-release, specifies "
            + "whether micro version shall "
            + "be incremented as well.",
           default=True,
        )

I could give it a try, when I'm home tomorrow.

@znichollscr
Copy link
Contributor Author

Nice. You might need to get rid of default=True too, time will tell

@carstencodes
Copy link
Owner

Unfortunately, the BooleanOptionalAction is still a bit buggy, but I managed it to setup a working implementation with a mutual exclusive argument group.

@carstencodes carstencodes marked this pull request as ready for review July 23, 2024 20:23
znichollscr and others added 2 commits July 23, 2024 22:24
It would also be possible to use store_const
and have different None handling, see e.g.
https://docs.python.org/3/library/argparse.html
Micro and no-micro can now be used.
Micro is the default, no-micro must explicitly enabled.

Both values can be set to false, IF old behavior shall be restored
@carstencodes carstencodes force-pushed the apply-intended-default-increment-micro-from-cli branch from 119350f to 8d744ef Compare July 23, 2024 20:25
@carstencodes carstencodes merged commit be6dc28 into carstencodes:main Jul 23, 2024
16 of 17 checks passed
@znichollscr
Copy link
Contributor Author

Seems to work in the quick test I did, thanks!

@znichollscr znichollscr deleted the apply-intended-default-increment-micro-from-cli branch July 26, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug? Bump pre-release from released version
2 participants