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

Do not issue deprecation warnings during import #244

Conversation

samueljsb
Copy link
Contributor

Prior to this change, the Unit enum would issue a deprecation warning
during import when its __dict__ was accessed. This is not correct: the
warning should only be issued if the enum is actually accessed.

This change ensures that no deprecation warnings are issued by the enum
during import.

Fixes #242.

e.g:

$ python -Werror
>>> from humanize import time
>>> time.Unit.DAYS
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/samuelsearles-bryant/dev/samueljsb/humanize/src/humanize/time.py", line 50, in __getattribute__
    warnings.warn(
DeprecationWarning: `Unit` has been deprecated. The enum is still available as the private member `_Unit`.

Prior to this change, the `Unit` enum would issue a deprecation warning
during import when its `__dict__` was accessed. This is not correct: the
warning should only be issued if the enum is actually accessed.

This change ensures that no deprecation warnings are issued by the enum
during import.
@samueljsb
Copy link
Contributor Author

@hugovk This fixes the reported bug, although it's a bit hacky.

I can't find any good advice for deprecating enums anywhere online.

I also considered checking whether the name began with __ to avoid issuing a warning when we access magic methods. This fails because Python also gets the mro() during import, so we'd have to ignore that too... and that becomes a game of whack-a-mole (and no less hacky).

@hugovk
Copy link
Collaborator

hugovk commented Nov 30, 2021

Thanks for checking!

How do you feel about this hacky solution?

One option is just to skip this one particular warning. The enum was added fairly recently (#104) so perhaps it's not being used.

@samueljsb
Copy link
Contributor Author

I think we want something in __getattribute__ - if anyone does happen to be using it then .-access is the most likely way they'll be getting at its members.

If we're going to remove the deprecation warnings (and the public aliases) soon then I think this is an acceptable short-term hack.

@hugovk
Copy link
Collaborator

hugovk commented Nov 30, 2021

Sounds reasonable. Please could you check the CI failures?

@hugovk hugovk added the changelog: Fixed For any bug fixes label Nov 30, 2021
@hugovk
Copy link
Collaborator

hugovk commented Jan 27, 2022

This is being rather tricky for limited benefit, so on balance let's just keep Unit and deprecate the rest. If you're busy I can have a look at this so we can get the release out. Thanks for all your work so far!

@hugovk
Copy link
Collaborator

hugovk commented Jan 30, 2022

Please see PR #252 to keep Unit.

@hugovk hugovk closed this in #252 Jan 30, 2022
@samueljsb
Copy link
Contributor Author

Hey sorry for disappearing on you! Thanks for handling this - I agree the enum is to much hassle to deprecate cleanly.

@hugovk
Copy link
Collaborator

hugovk commented Feb 4, 2022

All good!

The other deprecations have been released in 3.14.0. I'll give it a bit of time and then release 4.0.0 with removals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Fixed For any bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

humanize raises DeprecationWarning upon import
2 participants