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

Update pre-commit hooks and drop support for Python 3.6 & 3.7 #1109

Merged
merged 8 commits into from
Sep 22, 2023

Conversation

odelalleau
Copy link
Collaborator

@odelalleau odelalleau commented Jul 28, 2023

Fixes #1108

This PR updates pre-commit hooks to the latest current versions of isort / black / flake8 / mypy, and ensures we can run all these tools in the repository without any complaint (which also required dropping support for Python 3.6 & 3.7)

@odelalleau
Copy link
Collaborator Author

@omry any objection to bumping the min version requirement to 3.8? Recent versions of black, etc. have all moved to 3.8.

@odelalleau odelalleau requested a review from omry July 28, 2023 17:39
@odelalleau
Copy link
Collaborator Author

Converting to draft until the move to 3.8 is approved

@odelalleau odelalleau marked this pull request as draft July 28, 2023 17:40
@odelalleau
Copy link
Collaborator Author

@omry thoughts on enforcing Python >= 3.8?

@omry
Copy link
Owner

omry commented Aug 14, 2023

lgtm.
FYI: if you want my attention ping me on a different channel.

@omry omry requested a review from Jasha10 September 5, 2023 03:09
Comment on lines -638 to -643
if sys.version_info < (3, 7, 0): # pragma: no cover
typed_dict = hasattr(type_, "__base__") and type_.__base__ == Dict
return origin is Dict or type_ is Dict or typed_dict
else: # pragma: no cover
typed_dict = hasattr(type_, "__base__") and type_.__base__ == dict
return origin is dict or typed_dict
Copy link
Owner

@omry omry Sep 5, 2023

Choose a reason for hiding this comment

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

This gets into actual cleanup of 3.6 code.
@Jasha10 , did we officially removed support for it yet?

Copy link
Collaborator Author

@odelalleau odelalleau Sep 5, 2023

Choose a reason for hiding this comment

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

This gets into actual cleanup of 3.6 code.

When you said "lgtm" I thought you were saying "yes" to my question "thoughts on enforcing Python >= 3.8?"

@Jasha10 , did we officially removed support for it yet?

All official releases of OmegaConf / Hydra still support Python 3.6. I'm proposing to drop this support in the next release (because it will make our lives easier, and I doubt anyone still runing 3.6/3.7 would care about not being able to run the latest version).

Edit: actually Hydra already dropped support for 3.6

Just in case we would want to distinguish before vs. after dropping
support for Python 3.6 / 3.7.
@odelalleau odelalleau changed the title Update pre-commit hooks Update pre-commit hooks and drop support for Python 3.6 & 3.7 Sep 5, 2023
@odelalleau
Copy link
Collaborator Author

odelalleau commented Sep 5, 2023

Un-drafting as I believe it's ready now.

Next step on my side: doing something similar in Hydra (may take a few more weeks as I'm working on it quite sparingly).

Edit: actually Hydra already dropped 3.6 (facebookresearch/hydra#2523)

@odelalleau odelalleau marked this pull request as ready for review September 5, 2023 12:25
@odelalleau odelalleau requested a review from omry September 5, 2023 12:25
@odelalleau
Copy link
Collaborator Author

@Jasha10 & @omry I would like to get at least one official approval on the updated PR before merging -- thanks! (no rush)

Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review. This diff looks good to me!

@odelalleau odelalleau merged commit ceec6f4 into omry:master Sep 22, 2023
6 checks passed
@odelalleau odelalleau deleted the od/update-pre-commit-hooks branch September 22, 2023 12:24
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.

Pre-commit hooks are not working correctly
3 participants