Skip to content
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

modules: rename built-in plugins to "builtins" #2504

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Conversation

dgw
Copy link
Member

@dgw dgw commented Aug 21, 2023

Description

Finally does away with deadnaming core plugins as "modules". The alternative we were thinking of a while ago was sopel.internals (to mirror how custom plugins not part of a Python package emit logs under sopel.externals.*), but I liked builtins better than internals.

Updated relevant documentation examples and test code.

Have not renamed any functions/methods/attributes/variables in code that manipulates plugins from the sopel.modules (now sopel.builtins) module namespace.

Opening as draft first, since I made most of these modifications in a terminal emulator on my phone and have definitely not had a chance to double check everything yet. (Also the reason that half the checklist below is blank for now.)

Milestoned for 8.0.0 to start, but I'll be the first one to bump it into 8.1.0 if everything else is done but this isn't ready yet for any reason.

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 lint and make test)
  • I have tested the functionality of the things this change touches

Open question

Do we need to keep shims in sopel.modules in case custom plugins import from those, and set a timeline for removing them for real later (in 9.0, probably)? Or can we just call this a non-breaking change because the core plugins aren't part of our public API?

I know I'm in favor of just moving the files, without any shimming or deprecation cycle, but I'm curious if anyone disagrees with that position.

@dgw dgw added the Housekeeping Code cleanup, removal of deprecated stuff, etc. label Aug 21, 2023
@dgw dgw added this to the 8.0.0 milestone Aug 21, 2023
@Exirel
Copy link
Contributor

Exirel commented Aug 21, 2023

Or can we just call this a non-breaking change because the core plugins aren't part of our public API?

I think it's a breaking change if we really want to prevent breaking someone's heating system. Otherwise, I say we go for it.

Copy link
Contributor

@SnoopJ SnoopJ left a comment

Choose a reason for hiding this comment

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

LGTM, passes QA for me locally.

+1 to making it a breaking change. No sense in drawing out the removal of the old name even longer.

sopel/plugins/__init__.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor

Exirel commented Oct 28, 2023

Just a quick note to say that I fully approve of this PR btw.

Updated relevant plugin handling, documentation/examples, & test files.
@dgw dgw marked this pull request as ready for review October 31, 2023 00:53
@dgw
Copy link
Member Author

dgw commented Oct 31, 2023

Rebased on master to resolve merge conflicts and tweaked the find_internal_plugins() code after all.

Will most likely merge this next because it touches so damned many files; other open PRs can be manually merged (if approved) or rebased as necessary. This is just one of those situations where "the perfect merge opportunity" will never come. Big PRs cause conflicts, but it doesn't make sense to do this kind of thing piecemeal.

@dgw dgw merged commit 26d57fa into master Oct 31, 2023
13 checks passed
@dgw dgw deleted the modules-builtins branch October 31, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Housekeeping Code cleanup, removal of deprecated stuff, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants