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

support for ignoring args #115

Closed
wants to merge 6 commits into from
Closed

support for ignoring args #115

wants to merge 6 commits into from

Conversation

rainyl
Copy link

@rainyl rainyl commented Jul 31, 2023

Just do the same thing as #75 and #92 , but meet the format mentioned at #92 (comment)

for now, args can be ignored as follow:

# test.py

from tap import Tap, TapIgnore

class MyTapWithIgnore(Tap):
    arg_to_ignore_by_comment: str  # tap: ignore
    attr_to_ignore: TapIgnore[str]
    classvar_attr: TapIgnore[str]
    hello: str = "hello"

args = MyTapWithIgnore().parse_args()

print(args)

and the output:

python test.py -h
usage: test.py [--hello HELLO] [-h]

options:
  --hello HELLO  (str, default=hello)
  -h, --help     show this help message and exit
python test.py
{'hello': 'hello'}

By the way, some codes were borrowed from #92 and the official typing library.

@martinjm97
Copy link
Collaborator

Hi @rainyl,

Thank you for contributing! We appreciate you adding TapIgnore.

Prior to merging, we have a few concrete recommendations and questions.

For subscriptable types in a TapIgnore, the IDE does not seem to know that argignore: TapIgnore[int] means that argignore is of type int. Rather, it thinks it is of type Any. Is there a way to make this work and if not what's a good solution?
Screenshot 2023-08-02 at 3 48 20 PM

Ideally, there'd be a test case then ensures that TapIgnore typed arguments cannot be provided on the command line.

We're not familiar with the _SpecialForm. Is this the standard way to define a subscriptable type?

The current code seems to fail a test: ERROR tests/test_actions.py - TypeError: __init__() missing 1 required positional argument: 'doc'

We'll do our best to help out, but would love further contributions. Thank you again!

Best,
Kyle and Jesse

@rainyl
Copy link
Author

rainyl commented Aug 3, 2023

Hi, @martinjm97

Well, after working for some time, I used some hacking techniques to implement it on python version >=3.9, for 3.8 and former, the best solution seems to be using ClassVar directly.

TL;DR, regard TapIgnore as an alias to ClassVar, but with a name of "TapIgnore"

TapIgnore = ClassVar
TapIgnore._name = "TapIgnore"

Specifically, a user-defined type TapIgnored[T] different from types in typing lib will be regard as class and can't be treated as T directly, this feature must be supported by type checking library like pyright (or pylance, mypy, etc.) to extract specific T, you know, it's nearly impossible, at least for now. In the above code, hacking means it's not a standard and elegant implementation.

Finally, I think the best implementation is that:
for python 3.9+, use above solution.
for python 3.8-, give up supporting TapIgnore[T] -> T, use Generic[T] instead, hint in IDE will be shown as TapIgnored[int]

Also, If you have better solutions, please let me know :)

@rainyl
Copy link
Author

rainyl commented Aug 3, 2023

Ideally, there'd be a test case then ensures that TapIgnore typed arguments cannot be provided on the command line.

As for this, I find it had been solved.

For example, for the following code

from tap import Tap, TapIgnore

class MyTapWithIgnore(Tap):
    arg_to_ignore_by_comment: str  # tap: ignore
    attr_to_ignore: TapIgnore[str]
    hello: str = "hello"

args = MyTapWithIgnore().parse_args([
    "--hello", "hello world",
    # "--attr_to_ignore", "ignored",
    # "--arg_to_ignore_by_comment", "ignored by comment"
])

print(args)

and the output:

(venv38) > python .\test.py
usage: test.py [--hello HELLO] [-h]
test.py: error: unrecognized arguments: --attr_to_ignore ignored --arg_to_ignore_by_comment ignored by comment
(venv38) > python .\test.py
usage: test.py [--hello HELLO] [-h]
test.py: error: unrecognized arguments: --attr_to_ignore ignored
(venv38) > python .\test.py
{'hello': 'hello world'}

@martinjm97
Copy link
Collaborator

Current implementation. It seems that the current solution (on Python 3.10) improves the situation by making it so that the type is TapIgnore[int] rather than Any.

Screenshot 2023-09-06 at 4 44 08 PM

Ideally, we'd want it to be of type int.

Suggestion. The code below does seem to work for us (at least for Python 3.10)!

T = TypeVar('T')
TapIgnore: TypeAlias = ClassVar[T]
TapIgnore._name = 'TapIgnore'

What do you think? We set the _name field in the code above in the same way you did in your code. Why did you add that line? It doesn't seem to affect the behavior.

Thank you for keeping on working on this. We're very grateful.

Best,
Kyle and Jesse

@rainyl
Copy link
Author

rainyl commented Sep 7, 2023

Well, the codes you provided above won't work in VSCode.

T = TypeVar('T')
TapIgnore: TypeAlias = ClassVar[T]
TapIgnore._name = 'TapIgnore'

image

but this worked:

TapIgnore = ClassVar
TapIgnore._name = "TapIgnore"

image

However, this solution seems won't work in pycharm, which has it's own type checking tool.

As for TapIgnore._name="TapIgnore", It is because that, the name of TapIgnore with Tapignore=ClassVar will be ClassVar rather than Tapignore, and thus, the result of get_origin() will ClassVar.

Finally, I suggest that just use ClassVar to indicate ignored variables, the tricks above may not work in every type checking tool like pylance, mypy, and pycharm's, but they all support ClassVar.

@martinjm97
Copy link
Collaborator

Hi @rainyl,

Thank you again for the follow up! What an interesting problem. We weren't able to exactly reproduce the issue you observed, but we had a similar problem with VSCode until we installed mypy.

For Python 3.10.0, we used your test case above and looked at results in PyCharm and VSCode with TapIgnore defined as follows:

T = TypeVar('T')
TapIgnore: TypeAlias = ClassVar[T]
TapIgnore._name = 'TapIgnore'

VSCode version 1.82.1
Without mypy installed, it doesn't identify problems in type checking as you observed. However, with mypy installed, it seemed to work (it gave type TapIgnore[str] rather than str, but it still correctly identified the type mismatch and said it was between str and int rather than TapIgnore[str] and int). This isn't ideal, but it clears the bar for us in terms of practicality. We can add mypy as a dependency to resolve this problem for users.

Pycharm version 2023.1.1 (Community Edition)
Everything worked as expected.

Let us know if installing mypy works for you as well.

Thank you again,
JK (Jesse and Kyle)

@rainyl rainyl closed this Mar 10, 2024
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.

2 participants