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

Various cleanups; type hint fixes incl. corresponding to PEP 484 #185

Merged
merged 3 commits into from
Dec 12, 2022

Conversation

tomaarsen
Copy link
Member

Hello!

Pull request overview

  • Fix type hints in various locations, primarily adding Optional in accordance to PEP 484.
  • Various small cleanups:
    • Removing unnecessarily duplicate code
    • Using a backtick around class/function/variable names in logs and warnings more consistently.

Details

PEP 484 now states that a type hint of int = None is no longer correct. In previous versions, this would have been allowed, as it implicitly becomes Optional[int] = None. This was later changed, requiring us to be explicit with Optional.

Each of the changes are small, and for the most part ought to speak for themselves. Please let me know if you have any questions and feel free to review.

  • Tom Aarsen

See towards the bottom of https://peps.python.org/pep-0484/#union-types. It states that in past versions of the PEP, int = None would have been allowed, as it implicitly becomes Optional[int] = None. This is no longer the case, and we need to be explicit.
Copy link
Member

@lewtun lewtun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all these fixes @tomaarsen ❤️ ! They LGTM so happy to merge once we resolve the conflicts from some prior PRs getting merged

@tomaarsen
Copy link
Member Author

I'll get right on that!

@lewtun
Copy link
Member

lewtun commented Dec 12, 2022

Sorry to be a pain, but only just getting back to this now and there's a new round of merge conflicts to resolve. If you don't mind fixing them, I'll merge this right away!

@tomaarsen
Copy link
Member Author

It seems like we're experiencing some issues with the latest isort version that released some ~45 minutes ago, see https://github.com/huggingface/setfit/actions/runs/3679204969/jobs/6223374484 and PyCQA/isort#2031. I've prepared a PR on isort to resolve the issue: PyCQA/isort#2032.

@tomaarsen
Copy link
Member Author

tomaarsen commented Dec 12, 2022

isort has been updated, and so the CI is all green now.

@tomaarsen tomaarsen merged commit eee595e into huggingface:main Dec 12, 2022
@tomaarsen tomaarsen deleted the cleanups branch December 12, 2022 21:01
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