-
-
Notifications
You must be signed in to change notification settings - Fork 409
plugin: rename sopel.module into sopel.plugin #1898
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.
In addition to minimal feedback on the current PR contents, I looked for other files mentioning "module" that should probably be updated at the same time lest they get forgotten:
- contrib
- sopel.cfg (link to module-configuration page)
- docs
- source
- plugin.rst
- source
- sopel
- __init__.py
- coretasks.py
- irc
- backends.py
- modules
- check/update all
also look for "Module" instead of "Plugin" in the top-level docstring
- check/update all
- plugin.py (docstrings & examples that still refer to its old name)
- tools
- __init__.py
- _events.py
- jobs.py (I think)
- web.py (or skip it, since it's just a comment and the module is deprecated)
- test
- you already knew about these, of course
PRs like this remind me not to pay that much attention to GitHub's contributor stats, because depending on who does them, big renames like this or the one I did with web skew the numbers quite a bit. 😛
|
Great review, I updated docstrings & strings here and there. It's ready for review! |
|
This pull request introduces 1 alert when merging e961948 into 82ad1b3 - view on LGTM.com new alerts:
|
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.
Couple little details, mostly about the legacy imports left in sopel.module.
|
@dgw should be good now. |
|
This pull request introduces 1 alert when merging b379c70 into 73e7230 - view on LGTM.com new alerts:
|
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 gone through the changes since my last review, and decided they're Good Enough™. We already know there will be a follow-up PR to switch things over "for real" to the new plugin name and tweak some other stuff. Squash what you feel like, if anything, and let's
.
b379c70 to
adb4a31
Compare
|
Thanks. I'm happy with the rebase. Let's move forward (when Travis & LGTM are OK). |
|
This pull request introduces 1 alert when merging adb4a31 into a5a6e2d - view on LGTM.com new alerts:
|
|
1 suppressed* alert, you silly static-analysis tool. |
This is a hotfix, this code should have been modified by sopel-irc#1898 and not merged with this error. My bad.
Description
It partially implements the changes required by #1738 but contains only the very bare minimum changes.
I wanted to make sure that it is backward compatible. As a proof, I'll use the current test-suite.
The next steps will include updating Sopel's tests to use
sopel.plugin, and to change Sopel's built-in plugins to usesopel.pluginaswell.Note: I had to add
labelto the__all__magic constants to make it work properly. I guess I forgot that part back when I added this decorator.Edit: Updated the doc and docstrings, because it wouldn't work otherwise. I didn't try to keep old link to the doc of
sopel.module. I'm still working on a rewrite of the documentation related to plugins, so this will be probably discussed/taken care of at that point.Checklist
make qa(runsmake qualityandmake test)