-
-
Notifications
You must be signed in to change notification settings - Fork 409
sopel: move deprecated() to sopel.lifecycle #2232
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
Conversation
dgw
left a comment
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.
I've had another thought since we talked about this, regarding the new module name. What if we called it instead sopel.lifecycle?
At the very least, that makes more sense as a title for the doc page, and it means we could have other API lifecycle stuff in there later. Fancy a decorator for alerting users to planned function signature changes, for example? (That's a quick idea, so don't judge it too hard. 😝)
If you don't like this idea, see line notes for how to improve the docs for how things are in the PR at present.
|
I'm still looking for a general reaction to calling this module |
|
Hm. Lifecycle could be interesting if we want to add subclass of Well, I guess it's time for me to think about that a bit more. |
cf2ea28 to
b7ec2b6
Compare
|
Rebased & squashed because conflicts, and then I moved to |
dgw
left a comment
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.
Why does it feel like I didn't even look at the code before? Maybe you changed some of this stuff in the rename from sopel.deprecated to sopel.lifecycle, idk.
Also note, you'll want to either squash the second "renale" commit or fix that spelling error. 😸
|
Yup yup applied & fixed. No rebase nor squash yet, of course. |
dgw
left a comment
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.
Time to squash. Really wish there was a "squash" emoji; either the food or the sport would do.
The `deprecated()` function is used everywhere, whenever we need to deprecate something. Sometimes, that somethings is right where this function is defined: in `sopel.tools`, causing cyclic import errors. In an effort to reduce cyclic import errors, and to organize the code a bit better (mostly by removing everything from `sopel/tools/__init__.py`), this function is moved to its own sopel's submodule: `sopel.deprecated`. Documentation has been updated accordingly, and every part of Sopel that used `sopel.tools.deprecated` is now using `sopel.deprecated.deprecated`. In order to prevent yet another cyclic import error, `deprecated` no longer uses `sopel.tools.get_logger` to get the logger for an external plugins (that isn't using the namespace), but tries to mimic its behavior for namespace plugins only. Co-authored-by: dgw <[email protected]>
150c461 to
952e6b1
Compare
Description
The
deprecated()function is used everywhere, whenever we need to deprecate something. Sometimes, that somethings is right where this function is defined: insopel.tools, causing cyclic import errors.In an effort to reduce cyclic import errors, and to organize the code a bit better (mostly by removing everything from
sopel/tools/__init__.py), this function is moved to its own sopel's submodule:sopel.lifecycle.Documentation has been updated accordingly, and every part of Sopel that used
sopel.tools.deprecatedis now usingsopel.lifecycle.deprecated.In order to prevent yet another cyclic import error,
deprecatedno longer usessopel.tools.get_loggerto get the logger for an external plugins (that isn't using the namespace), but tries to mimic its behavior for namespace plugins only.Edit: After discussion, I moved from
sopel.deprecatedtosopel.lifecycle. This module can be used, in the future to manage more version & lifecycle related things. But for now, I'd rather see this merged than trying to do too much and be frustrated because I've other things in mind that are blocked by this change.Checklist
make qa(runsmake qualityandmake test)