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

[proposal] Implement simplified version of pyutilib #7976

Merged
merged 8 commits into from
May 14, 2024
Merged

Conversation

smotornyuk
Copy link
Member

Replace PyUtilib dependency with the simplified version of its logic written inside CKAN.
It can help everyone to understand how plugins work(because I'm keeping only bare minimum that is actually used by CKAN plugins) and decide if we can profit from migration to existing plugin-framework, like pluggy

@EricSoroos, I know that you've started porting pyutilib.core into CKAN so I want you, as person who probably knows most of pyutilib internals, to decide if this PR has any sense. I spent just a couple of hours on it and I feel no regrets if it's closed:)

@smotornyuk smotornyuk marked this pull request as draft December 12, 2023 12:42
@smotornyuk smotornyuk marked this pull request as ready for review April 17, 2024 22:18
@smotornyuk
Copy link
Member Author

Implementation finished. Plugins that used only p.implements(INTERFACE, inherit=True) and did not call any internal methods of the SingletonPlugin/Interface are completely compatible with the new version.

In addition, I made it possible to extend interfaces

class Plugin(p.SingletonPlugin):
    p.implements(p.IClick, inherit=True)

### or
class Plugin(p.SingletonPlugin, IClick):
    pass

The second form plays better with analysis tools because it does not perform any magic behind the scenes. You'll get a notification about incorrect method implementations from your IDE, as well as better autocompletion. There is a bad side. Extending the interface means you inherit all the default implementations of the methods from the interface. I.e, it's identical to p.implements(..., inherit=True).
In certain scenarios, one may want some of the interface methods missing. I cannot imagine the use case, but I certainly saw this before. This workflow is not available when an interface is extended.

Nevertheless, I'd recommend extending over our standard p.implements because of the profits above. On top of this, if p.implements will be deprecated, we can remove the PluginMeta metaclass and get rid of its magic completely, leaving just a small simple set of normal classes.

@amercader amercader added this to the CKAN 2.11 milestone Apr 26, 2024
@@ -724,7 +724,7 @@ def _group_or_org_create(context: Context,
group_type = data_dict.get('type', 'organization' if is_org else 'group')
group_plugin = lib_plugins.lookup_group_plugin(group_type)
try:
schema: Schema = group_plugin.form_to_db_schema_options({
schema: Schema = getattr(group_plugin, "form_to_db_schema_options")({
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why are these calls changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here group_plugin is an instance of IGroupForm according to the type signature of functions. This interface no longer has form_to_db_schema_options and the whole try/except block that wraps the current change exists for backward compatibility.

With a simplified implementation of the plugin system, the type checker now can detect this issue and report a call to the non-existing method form_to_db_schema_options. So I have to either add the type: ignore directive or modify the line to verify, that I know what am I doing.

I decided to use getattr, because unlike type: ignore, it highlights the unsafe expression and now it's more clear, what exactly can cause an AttributeError that we are catching in try/except

@amercader
Copy link
Member

This looks good to me

@smotornyuk did I get all the items in the changelog right?

https://github.com/ckan/ckan/pull/7976/files#diff-ee790801a4c49e06c15ca11eab2a0e65a88a759f20218ad9de86f1094e9983aa

@smotornyuk
Copy link
Member Author

Yes, everything is there

@amercader amercader merged commit 15f32e5 into master May 14, 2024
8 checks passed
@amercader amercader deleted the remove-pyutillib branch May 14, 2024 09:56
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.

None yet

4 participants