Add Positional Feature#176
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
==========================================
+ Coverage 94.18% 94.38% +0.20%
==========================================
Files 4 4
Lines 757 784 +27
==========================================
+ Hits 713 740 +27
Misses 44 44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| kwargs["help"] += " " + self.class_variables[variable]["comment"] | ||
| if variable in self.class_variables and (comment := self.class_variables[variable]["comment"]): | ||
| kwargs["help"] += " " + comment | ||
|
|
There was a problem hiding this comment.
This avoids just an additional untrimmed space at the end of the help message if there is none; i.e. "(int, required) " -> "(int, required)"
Refactor tests to use updated function signatures and handle optional parameters correctly.
|
Ready for review :) |
|
Thank you! This looks great! Sorry for the delay, we'll do our best to review this sizable PR soon. --JK |
|
Tell me if you think something needs a redesign. |
swansonk14
left a comment
There was a problem hiding this comment.
Hi @Daraan,
Thank you so much for this PR! Overall it looks great! One issue we noticed is a mismatch in behavior between argparse and Tap in the way that the order of positionals with defaults is handled. We will work on fixing it in the next few hours. We also left a few other comments regarding additional tests and documentation that we will work on.
--JK
| if PositionalOptional is not PositionalOptionalOrder2: | ||
| args2 = tapped2.parse_args(["custom"]) | ||
| self.assertEqual(args2.arg, "custom") | ||
| self.assertEqual(args2.barg, 1) |
There was a problem hiding this comment.
We agree that this is correct behavior for PositionalOptionalOrder1, but we think that this is incorrect for PositionalOptionalOrder3. In PositionalOptionalOrder3, since barg is the first positional, the input "custom" should map to barg even though barg has a default value, which would then lead to a crash. This happens in argparse as seen in the code below.
from argparse import ArgumentParser
parser = ArgumentParser()
parser.add_argument("barg", type=int, default=1)
parser.add_argument("arg", type=str)
args = parser.parse_args(["custom"])which produces the error
usage: main.py [-h] barg arg
main.py: error: argument barg: invalid int value: 'custom'
We would suggest changing this test so that PositionalOptionalOrder1 runs this code but PositionalOptionalOrder3 expects a SystemExit.
| def greet(name: Positional[str]): | ||
| return f"Hello, {name}!" |
There was a problem hiding this comment.
These tests are great, but it might be worth adding at least one more complex test with multiple positional and non-positional arguments.
There was a problem hiding this comment.
It would also be good to include a test that passes an argument to the function directly (via **func_kwargs in tapify) rather than through the CLI. For example,
def greet(name: Positional[str], age: int):
return f"Hello, {name} ({age})!"
output = tapify(greet, command_line_args=["Alice"], age=32)| class MyTap(Tap): | ||
| required_arg: str | ||
| default_arg: str = 'default value' | ||
| positional_arg: Positional[int] |
There was a problem hiding this comment.
Show example usage.
"""main.py"""
from tap import Tap, Positional
class MyTap(Tap):
required_arg: str
default_arg: str = 'default value'
positional_arg: Positional[int]
args = MyTap().parse_args()
print(f"positional = {args.positional_arg}, required = {args.required_arg}, default = {args.default_arg}")For example, running
>>> python main.py 42 --required_arg "I'm required"would produce
positional = 42, required = "I'm required", default = "default value"|
We believe we have the few minor fixes completed. We'll merge and then apply those fixes. Thank you again and sorry for the delay on the merge! --JK |
Resolves: #169
To also work for pydantic needs: #174, then also resolves: #165 (thats why latest tests) fail
Supports