tools: deprecations & reorganization (feat. sopel.cli)
#2385
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This looks like a remnant of Sopel's old (think 3.x days) logging setup. We are WAY past that, and even the one plugin I thought might be abusing it, sopel-ipython, doesn't.
A plugin isn't going to need `check_pid()`. The only usage of it at present is within `sopel.cli`, so it definitely fits better under `sopel.cli.utils`. For `tools.stderr()`, ideally people are using a logger and do not print directly to the terminal from a plugin. (And if they want to have their plugin directly print to the terminal, removing the convenience function from `sopel.tools` makes doing stuff The Wrong Way™ just that little bit more annoying, which is good.) Finally, `tools.get_input()` was already marked as deprecated but it hadn't been replaced yet in real code with the built-in `input()`. Being Python-3-only now, we don't have to worry about the difference between what `input()` does in py2 and py3. (I started out doing these in separate commits, but ran into too many circular-import errors and eventually decided to just do ALL the things in one go.)
dgw
added
Breaking Change
SnoopJ
approved these changes
Dec 11, 2022
Contributor
SnoopJ
left a comment
SnoopJ
left a comment
There was a problem hiding this comment.
Pretty straightforward reorganization, LGTM 👍
Exirel
approved these changes
Dec 22, 2022
Contributor
Exirel
left a comment
Exirel
left a comment
There was a problem hiding this comment.
Let's deprecate everything. 😈
This was referenced Jan 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Started looking at
sopel.toolsas a follow-up to #2379 and noticed some more cleanup that could be done. Specifically:OutputRedirectclass is used by nothing, appears to be a relic of the 3.x-era logging system, and should be deprecated.check_pid()belongs under theclinamespace, because that's the only code that needs it. And plugins shouldn't be touching PIDs anyway.stderr()also ideally should never be used by plugins. If they really really want to print directly to a terminal instead of using a logger, they have access toimport sys; print('some bs', file=sys.stderr)on their own.get_input()is already deprecated, but I had to replace uses of it to eliminate circular imports (because I'm perhaps foolishly keeping the moved functions available intoolsuntil 8.1).Checklist
make qa(runsmake qualityandmake test)Notes
I was going to do this stuff a bit more atomically, moving things from
toolstocliin a series of several commits, but that approach ran into circular-import errors if I tried to only move one function at a time. So I'm sorry, but I couldn't find a reasonable way to break it down into smaller commits. You'll just have to review the whole thing at once. 🙏