Closed gutzbenj closed 3 years ago
Hi there! Thank you for your nice words!
It seems you caught the first bug. Thank you for reporting it! The bug affects the &
and |
operator of Predicate
and it's very easy to fix. Curiously enough, it would not exist if I had not forgotten to write a type annotation ^.^".
I'm gonna write a fix and include it in v0.8.0, which should come out in the next days, probably tomorrow.
Is this the recommend way of setting the constraints?
Regarding what's reccomended, it depends... there are many ways to define a constraint:
constraint
to @option_group
as you did@constraint
click.UsageError
with a manually written error when something is wrong.If you have a complex one-time constraint that generates an unclear / non-optimal message (so you need to rephrase it with rephrased()
), the advantage of using Cloup constraints over writing custom logic diminishes.
Both your constraints are formally correct but of course not equivalent. If latitude and longitude must always be provided, then you should definitely make them required
.
I may help you in finding the optimal solution if you answer these questions:
cloup.option("--coordinates", metavar='LATITUDE LONGITUDE', nargs=2, type=click.FLOAT),
Actually my thought solution is not working so I'll have to find another way.
The options --latitude and --longitude should only be provided within the geo filtering option, because they are not needed when e.g. filtering for a station name. However when setting them to required within the option group they are always required whether or not I want to apply a geo filter.
My favorable solution is as given in the first codeblock because everything would be closed and wrapped up in one option group, arguments/options as well as constraints.
Yes, I suspected that making coordinates required was not what you wanted.
The problem I have with your first solution is that the generated help string and the error messages are not really clear. I mean, this is not how I would explain the constraint to another human:
exactly 3 required if --latitude is set and --longitude is set
Of course, you can use rephrased()
to change the help text but changing error messages is more problematic with conditional constraints because they have two branches (and conditions cannot be rephrased now).
(By the way, I'll probably introduce an AllSet(*params)
predicate so the condition description will be "--latitude and --longitude are set".)
In my opinion, you get better help text and errors if you split the constraint in two:
latitude
and longitude
go together, never alonerank
and distance
.So I would suggest one of the following solutions that will work fine in v0.8.0.
import click
import cloup
from cloup.constraints import If, IsSet, RequireExactly, accept_none, all_or_none
"""
Solution 1
"""
@cloup.command()
@cloup.option_group(
"Latitude-Longitude rank filtering",
cloup.option("--latitude", type=click.FLOAT),
cloup.option("--longitude", type=click.FLOAT),
cloup.option("--rank", type=click.INT),
cloup.option("--distance", type=click.FLOAT),
help="Provide the coordinates plus either --rank or --distance.",
)
@cloup.constraint(all_or_none, ['latitude', 'longitude'])
@cloup.constraint(
If(['latitude', 'longitude'],
then=RequireExactly(1),
else_=accept_none),
['rank', 'distance']
)
def solution_one(**kwargs):
pass
"""
Solution 2
--coordinates instead of --latitude and --longitude
"""
@cloup.command()
@cloup.option_group(
"Latitude-Longitude rank filtering",
cloup.option("--coordinates", metavar='LATITUDE LONGITUDE',
nargs=2, type=click.FLOAT),
cloup.option("--rank", type=click.INT),
cloup.option("--distance", type=click.FLOAT),
help="Provide --coordinates plus either --rank or --distance.",
)
@cloup.constraint(
If('coordinates', then=RequireExactly(1), else_=accept_none),
['rank', 'distance']
)
def solution_two(**kwargs):
pass
In both cases, I used the option group help to provide instructions to the user. In think this is more clear than showing the costraints section (show_constraints=True
) in this case.
Dear @janLuke ,
thank you very much for this precise description! I'll be happy using v0.8.0! Thanks and have a nice weekend! :)
You're welcome! Have a nice weekend too!
(I'm reopening the issue. I'll close it when it's fixed.)
I decided to ship this fix and other minor fix/improvements in a patch release (v0.7.1
). Of course, this doesn't include any new feature like the AllSet
condition.
I'm gonna release v0.8.0 very soon.
(v0.8.0 is finally out)
Description
Dear @janLuke ,
I am currently working on a switchover for our library wetterdienst [1]. Therewith I'm currently moving our client accessor to click, however as we have certain mixing arguments for some functions I was happy to find that you are working on making this easier allowing to check for conditions. First of all thanks for your much needed work!
I'd like to add the following condition where the user can either supply rank or distance which in combination with provided longitude and latitude is used for geo filtering of some weather stations.
However as far as it goes it would only give me a nice help text but when running the command with arguments it ends up with
Is there a way to achieve what I'm planning with the current status of the library?
Again, thanks a lot for your work here! Much appreciated!
[1] https://github.com/earthobservations/wetterdienst
Edit Got it working by making the first two arguments required like
Is this the recommend way of setting the constraints?