-
Notifications
You must be signed in to change notification settings - Fork 26.3k
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 PEP 563 for HfArgumentParser #15795
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
e13b2dc
to
2eb6951
Compare
c82b88d
to
16aef16
Compare
kwargs["nargs"] = "?" | ||
# This is the value that will get picked if we do --field_name (without value) | ||
kwargs["const"] = True | ||
elif isclass(origin_type) and issubclass(origin_type, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative implementation without regular expression for #15951
@LysandreJik Hello, would be great if you could help or ask someone to give any feedback on this, thanks! |
Looking forward to any feedback. |
It looks like you did a rebase on your PR that GitHub did not appreciate (the diff shows 292 files). Could you close this PR and open a fresh one? |
ac35ab1
to
d47e068
Compare
@sgugger Github encountered some internal error a little while ago, it seems to be fine now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this! The diff is indeed more readable now.
I just have one comment to make the code more readable, otherwise LGTM!
The documentation is not available anymore as the PR was closed or merged. |
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
@sgugger Thanks so much for the feedback! I've adopted that. In addition, I've replaced |
Cool, thanks again for your contribution! |
* Support PEP 563 for HfArgumentParser * Fix issues for Python 3.6 * Add test for string literal annotation for HfArgumentParser * Remove wrong comment * Fix typo * Improve code readability Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * Use `isinstance` to compare types to pass quality check * Fix style Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
* Support PEP 563 for HfArgumentParser * Fix issues for Python 3.6 * Add test for string literal annotation for HfArgumentParser * Remove wrong comment * Fix typo * Improve code readability Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * Use `isinstance` to compare types to pass quality check * Fix style Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
What does this PR do?
Fixes #15739, support for PEP 563 is added, making it possible to write
from __future__ import annotations
in the file of dataclasses forHfArgumentParser
. But this will not be available for Python with a version lower than 3.7, thus addingfrom __future__ import annotations
in the test is currently infeasible because__future__
imports should always occur at the beginning of the file. Instead, I write a test case with string literal annotations.Before submitting
Who can review?
@LysandreJik