Skip to content

feat(plugins): Switch to an importlib.metadata.EntryPoint-based plugin loading #632

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

Merged
merged 1 commit into from
Dec 29, 2022

Conversation

noirbizarre
Copy link
Member

@noirbizarre noirbizarre commented Dec 5, 2022

Description

This PR is proposal implementation for #495

It switches to an importlib.metadata.EntryPoint-based plugin loading instead of pattern matching on the module name.
Plugins are now loaded using the commitizen.plugin entrypoint while legacy plugins are not loaded anymore but a warning is raised for each legacy plugin seen.

BREAKING CHANGE: Plugins are now exposed as commitizen.plugin entrypoints

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

Internal plugins are exposed as commitizen.plugin entrypoint. and seen as plugins.
External plugins are seen too as soon as they are exposed as commitizen.plugin entrypoint.
Legacy plugins are not loaded and a warning is raised for each legacy plugin detected.

Steps to Test This Pull Request

Try to use any internal plugin without changes in configuration: should work
Try a legacy plugin: KO
Try a plugin migrated to the commitizen.plugin: should work

Additional context

I chose not to have a deprecation warning on this because:

  • there was issues with the cz_ naming
  • handling plugins with both discover_this and entrypoint expose raise a bit the complexity

But I can switch to this if required:

  • register legacy plugins first while raising a deprecation warning
  • register entrypoints based plugins, overwritting those registered by legacy pattern
  • Add a deprecation warning in the documentation with a target version for support removal

Fixes #495

@woile
Copy link
Member

woile commented Dec 6, 2022

Fantastic PR! Thanks a lot for the contribution.

Could you add some documentation? As you can see this would be outdated: https://commitizen-tools.github.io/commitizen/customization/#2-customize-through-customizing-a-class

And if possible a migration guide from the old system, on that same page.

@noirbizarre
Copy link
Member Author

Sorry, the branch wasn't up to date and was missing the documentation part.
Here is the up to date branch.
I'll add a migration guide

@noirbizarre
Copy link
Member Author

There it is, migration section added 👍🏼

@Lee-W
Copy link
Member

Lee-W commented Dec 9, 2022

Looks like we'll need to wait for #633 for the CI ?

@noirbizarre
Copy link
Member Author

Yes, as soon as #633 is merged, I'll rebase this PR to pass the CI (I also have some other PR not yet submitted)

@noirbizarre noirbizarre changed the base branch from master to v3 December 13, 2022 09:39
@noirbizarre noirbizarre force-pushed the feature/entrypoints branch 4 times, most recently from 91ca554 to 741339f Compare December 13, 2022 11:03
@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

❗ No coverage uploaded for pull request base (v3@61c3024). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@          Coverage Diff          @@
##             v3     #632   +/-   ##
=====================================
  Coverage      ?   98.42%           
=====================================
  Files         ?       39           
  Lines         ?     1650           
  Branches      ?        0           
=====================================
  Hits          ?     1624           
  Misses        ?       26           
  Partials      ?        0           
Flag Coverage Δ
unittests 98.42% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@noirbizarre noirbizarre force-pushed the feature/entrypoints branch 5 times, most recently from 803740c to d5048ef Compare December 13, 2022 12:20
@noirbizarre
Copy link
Member Author

Retargetted and rebased on v3 branch. CI is passing 🎉

importlib_metadata is used everywhere, even for Python versions having it builtin because:

  • each builtin version currently have a different API (ongoing refactoring)
  • MyPy does not handle typing properly (making tests pass but fail on typing validation)
  • this avoid having a _compat module introducing behavorial and typing variations between supported Python versions

…n loading

Plugins are now loaded using the `commitizen.plugin` entrypoint
while legacy plugin are not loaded anymore but a warning is raised when one is seen.

Fixes commitizen-tools#495

BREAKING CHANGE: Plugins are now exposed as `commitizen.plugin` entrypoints
@noirbizarre noirbizarre mentioned this pull request Dec 28, 2022
4 tasks
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

This mechanism looks so great! @woile I plan to merge it later this week. Let me know if you want to take a look.

Copy link
Member

@woile woile left a comment

Choose a reason for hiding this comment

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

LGTM thanks a lot 💪🏻

@Lee-W Lee-W merged commit 5dfa0ef into commitizen-tools:v3 Dec 29, 2022
@noirbizarre noirbizarre deleted the feature/entrypoints branch December 29, 2022 14:00
@Lee-W
Copy link
Member

Lee-W commented Jan 18, 2023

Hi @noirbizarre I'm trying to install the plugins and test it locally. How did you do it on your local side?

@noirbizarre
Copy link
Member Author

It depends:

  • for commitizen built-in plugins, nothing to do except install commitizen. PLugins have been mapped to the same names
  • for external plugins, they need to be ported as described here (this is quick and easy) and installed (metadata are only visible for installed packages)

I did the migration on https://github.com/noirbizarre/emotional but did not yet push because v3 is not released (but I will, so you can test)

@Lee-W
Copy link
Member

Lee-W commented Jan 21, 2023

It depends:

* for `commitizen` built-in plugins, nothing to do except install `commitizen`. PLugins have been mapped to the same names

* for external plugins, they need to be ported as described [here](https://github.com/commitizen-tools/commitizen/blob/v3/docs/customization.md#migrating-from-legacy-plugin-format) (this is quick and easy) and installed (metadata are only visible for installed packages)

I did the migration on https://github.com/noirbizarre/emotional but did not yet push because v3 is not released (but I will, so you can test)

What I was trying is to rebase the latest master back to v3 but encounter commitizen.exceptions.NoCommitizenFoundException: The committer has not been found in the system. error. Woundring is there anything I did wrong?

I somehow solve it on my side. Just pushed the latest v3 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new plugin system
3 participants