Skip to content

Conversation

@half-duplex
Copy link
Member

Description

Exposes plugin._module.__version__ (or None) in plugin metadata, and adds .version plugin_name.

<!mal> .version
< Sopel> [version] Sopel v8.0.0.dev0 | Python: 3.9.5 | Commit: blahblah
<!mal> .version coretasks
< Sopel> [version] coretasks v8.0.0.dev0 (built in)
<!mal> .version mypackage
< Sopel> [version] mypackage v9.9.9
<!mal> .version test
< Sopel> [version] test (version unknown)
<!mal> .version fake
< Sopel> [version] I don't have a plugin named 'fake' loaded.

This will confuse any daring person with a sopel module named sopel, but that's a risk I'm willing to take.
It does work for individual .py files that contain a __version__.

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

@half-duplex half-duplex added this to the 8.0.0 milestone Jul 2, 2021
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Style nitpicks big and small. Which means they're not really all "nitpicks", I guess.

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Looks good but for a couple of docstring-formatting nits that I missed the first time. (In rST, a single ` does something entirely different from what it does in Markdown; the equivalent is ``.)

I don't care enough about the performance to benchmark .format() vs. %s interpolation vs. .join() or anything, and it's not like this is going to run on every line, so I just marked that thread as resolved. 🙃

@Exirel
Copy link
Contributor

Exirel commented Aug 16, 2021

At first, I was rather hesitant regarding plugin version, because something didn't sit well with me. Now, I think plugin's version is an interesting information, so I put that aside for later.

After further consideration, I think it would be best if the Plugin Handler object had a get_version method, in order to override how we get the version number based on the plugin type. All the code used to extract the version from the meta-data would be gone and replaced by using the clean interface.

@dgw
Copy link
Member

dgw commented Sep 18, 2021

@Exirel Is that "would be best" a request-changes or someone-should-follow-up-on-this? I'm ready to approve and let a plugin-handler get_version() method happen separately, fwiw.

@half-duplex Note that when you squash, it would be a good time to fix the typo "retreiving" in 1be8bbe

@half-duplex
Copy link
Member Author

half-duplex commented Sep 18, 2021

Exi, what's the benefit of a .get_version() over just changing how get_meta_description() generates the version return attr? If you prefer it on its own, would a property be cleaner?

@Exirel
Copy link
Contributor

Exirel commented Sep 18, 2021

someone-should-follow-up-on-this?

I think I left this comment expecting a conversation, and that didn't happen a month ago.

what's the benefit of a .get_version()

Getting a plugin's version can depends on the type of plugin, therefore each Plugin Handler may require an override on how to get said version. For example, there is no need for a __version__ attribute in an Entry Point plugin, since you can get that information from pkg_resource or setuptools (I don't remember the details). Therefore, having one method that does that is more convenient.

Another reason is that it's easier to get a Plugin object, and ask for its version, without having to retrieve a dict and hope that it gets everything required. It means less code overall, so less chance of mistake.

Last but not least, it's easier to deprecate a behavior if you have a method with a single purpose for it. That's the reason for a proper interface.

@dgw
Copy link
Member

dgw commented Oct 21, 2021

Still hoping for a refactor with that getter method Exi asked about.

@half-duplex
Copy link
Member Author

Now depends on #2199

@dgw
Copy link
Member

dgw commented Dec 12, 2021

Refocusing on some 8.0 stuff now that 7.1.7 is out (and we weathered the Python 3.9.8 kerfuffle). Merged #2199, which opens the way to rebasing this patch on top of it.

Looks like some small difference in documentation was introduced when splitting the new bot.plugins property into its own PR?
image

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Whee

@dgw dgw merged commit e4b80be into sopel-irc:master Dec 18, 2021
@half-duplex half-duplex deleted the plugin-version branch February 17, 2022 22:53
@half-duplex half-duplex restored the plugin-version branch May 14, 2023 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants