Skip to content

Conversation

@dgw
Copy link
Member

@dgw dgw commented Oct 18, 2020

Description

This is the final step for 7.1 in #1738. Making these commits was literally just me going through about 1,400 find-in-files results for the string "module". Fortunately I was able to skip a lot with negative regex lookarounds (e.g. decorators, object attributes).

I mixed in a few little tweaks to other stuff, because without them it would have been a very small PR indeed.

In summary:

  • coretasks: audit for outdated uses of "module" vs. "plugin" (6fa4bb1)
  • irc: audit for outdated use of "module" vs. "plugin" (650e672)
    • Some of this actually changes an argument name, something we might not want to do right now (and in fact the "module" moniker goes all the way down into the CapReq type from irc.utils).
    • More importantly, it seems that nothing actually uses the cap_req() function provided here. Something to address in a future patch, perhaps.
  • irc.utils: fix docs for CapReq param prefix (f8ed9e5)
    • The empty string is also allowed; it means "request this if no other CapReq forbids it".
    • Here I did not change the parameter name module or anything else, yet. I'm still not entirely sure how this and cap_req() fit together, and we'll probably end up dropping most or all of the above commit.
  • logger: decorate get_logger() with deprecation info (e02a286)
  • plugins: audit docs for outdated use of "module" vs. "plugin" (d5854f8)
  • plugins.handlers: small fixes (all uses of "module" are correct) (850a178)
  • irc.utils: rename "module" param & attribute to "plugin" (33b094b)
    • Kept CapReq.module as a @tools.deprecated @property. Looks like nothing was accessing it, but let's still keep it around until 8.0 just in case.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches
    • Note: There are minimal code changes (none if the single code-touching commit to irc is rejected)

Notes

Yes, I'm aware that I threw some unrelated stuff in here, not that long after admonishing @Exirel for doing the same in #1944. I don't regret it. That stuff needs fixing, too. 😜

dgw added 6 commits October 18, 2020 00:47
I'm doing this because it's easier to drop the change later than skip
over it now and then remember to come back later. There's a good chance
that we don't actually want to change most of this right now.

Oddly, it seems that nothing actually *uses* the `cap_req()` function
provided here. Something to address in a future patch, perhaps.
The empty string is also allowed; it means "request this if no other
CapReq forbids it".
It won't start emitting any warnings until 8.0, but we won't have to
remember to decorate it later thanks to the new `warning_in` parameter
added to `tools.deprecated` a while back.
@dgw dgw added this to the 7.1.0 milestone Oct 18, 2020
@dgw dgw requested a review from a team October 18, 2020 06:28
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

The old "module" attr will stick around as a deprecated property until
Sopel 8.0. Until then, any code that still uses it will get a nice big
warning printed in the logs. (I couldn't find any code that actually
accessed the "module" attribute of a CapReq object, but better safe.)
@dgw
Copy link
Member Author

dgw commented Oct 18, 2020

@Exirel You still OK with this, including the new commit I just added (33b094b)? The "deprecated property" solution was super obvious but I was so distracted by other stuff when putting together the rest of this PR that I didn't think of it. 😅

@Exirel
Copy link
Contributor

Exirel commented Oct 18, 2020

You still OK with this, including the new commit I just added

Yes, all good to me!

@dgw dgw merged commit fc7059e into master Dec 6, 2020
@dgw dgw deleted the audit-for-module-term branch December 6, 2020 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants