-
Notifications
You must be signed in to change notification settings - Fork 519
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
Add deprecate module #1416
Add deprecate module #1416
Conversation
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.
about the warning - ok
about the assert on version, that is incorrect.
before version release, we bump the __version__
, at wich point the tests will fail.
Added
QuestionThe function was renamed to
I changed the code accordingly, and I tested it - see added tests here a210e0a I think this type of exception should be directly implemented/checked in the test, and not to the user code (similarly to what we did with breaking changes) |
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.
LGTM
Left a minor comment on argument names. Not a big deal tho
Up to you whether to address or ignore if you disagree ;)
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.
LGTM
consider @BloodAxe comments about param names
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.
LGTM
Breaking change fail will be fixed in #1420
Please review the PR above first, to fix breacking change test 🙏
Motivation
Have a nicer decorator to deprecate functions. The idea is to easily deprecate any function, with nice logs
Answers to Ofri
I. Usage of
warning
vslogger.warning
With warnings, we get
DeprecationWarning
built-in, which is standard in python, and which is also easy to catch (see tests)fully_configured_deprecated_func()
in the example below. This is thanks tostacklevel
inwarnings.warn(message, DeprecationWarning, stacklevel=2)
I think using this is cleaner than
logger.warning
, what do you think ?II. Making sure that
removed_in_v
is enforcedAfter discussion with @shaydeci and @ofrimasad, we decided to enforce
removed_in_v
. Basically, using a function that is not supported (version > removed_in_v) will lead to an ImportError with a clear error message explaining how to fix it.Should be something like
I think it is eventually a nice way to remove a function and force people to move to the new one, while giving them a clear tip on how to do