search

labelle-org / labelle

Label printing software
Apache License 2.0
50 stars 8 forks source link

Fix --fixed-length Flag Doesn't Work Alone #79

Closed poita66 closed 1 month ago

poita66 commented 1 month ago

Fixes https://github.com/labelle-org/labelle/issues/78

Using the --fixed-length flag currently yields the following error:

Usage: python -m labelle.cli.cli [OPTIONS] [TEXT]... COMMAND [ARGS]...
Try 'python -m labelle.cli.cli --help' for help.
╭─ Error ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ Invalid value: Cannot specify min/max and fixed length at the same time                                                                                                                                      │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

This PR allows the --min-length flag to be 0 or None

maresb commented 1 month ago

Thanks for catching this! Can't we get rid of the min_length == 0 check?

poita66 commented 1 month ago

That's what I originally did, but thought that we should retain the existing ability to provide both --min-length and --fixed-length in case anyone was already using them together. This way it's not a breaking change.

maresb commented 1 month ago

Ah ya, I see your point. But in this case I think it was a pure bug. Semantically it doesn't make sense to have --min-length=0 and --fixed-length=x. I'd rather just fix it and consider it as a "bugfix" rather than a "breaking change". Given the error message I think it should be straightforward for anyone who used the --min-length=0 to figure out, but honestly you're probably the first (or at worst second) person to hit this.

poita66 commented 1 month ago

Yeah, fair. I'll update the PR

poita66 commented 1 month ago

Sorry for the mess of commits. I did them on my phone, which was a mistake. I'll squash them on my laptop in the morning.

maresb commented 1 month ago

No worries and no rush! Thanks so much for the contribution!

maresb commented 1 month ago

This looks good to me, so I'm just going to squash-merge it. Thanks!!!