chore!: restructure plugin models #775

lengau · 2024-07-10T15:50:43Z

Have you signed the CLA?

This does several related things:

Removes the vestigial PluginModel from craft_parts.plugins.base
Combines PluginPropertiesModel and PluginProperties (which didn't need separation anymore)
Makes the unmarshal method of PluginProperties usable by all plugins (and deletes the unnecessary overrides)

This class was vestigial, being a child of PluginPropertiesModel, while all plugins' properties classes are children of PluginProperties, which is itself a child of PluginPropertiesModel. This is a breaking change because external plugins for applications (e.g. snapcraft) inherit from both PluginProperties and PluginModel.

This does several things: - Merges PluginProperties and PluginPropertiesModel - Makes PluginProperties.unmarshal() work for most plugins be default - Makes PluginProperties.marshal() fully dump json-able objects - Adds a `plugin` field to all plugins, providing their names.

sergiusens · 2024-07-11T01:45:36Z

craft_parts/plugins/properties.py

+        validate_assignment=True,
+    )
+
+    plugin: Any = ""


Is it to play nice with the various Literal["x"] overrides in subclasses?

It was left over from what @tigarmo suggests when I was playing around with different ways to make creating child nicer and cleaner with pyright. In the end I went with double-declaring frozen=True though, so I'll switch this back to str

sergiusens · 2024-07-11T01:47:55Z

craft_parts/plugins/properties.py

@@ -65,4 +81,7 @@ def get_pull_properties(cls) -> list[str]:
    def get_build_properties(cls) -> list[str]:
        """Obtain the list of properties affecting the build stage."""
        properties = cls.schema(by_alias=True).get("properties")
-        return list(properties.keys()) if properties else []
+        if properties:
+            del properties["plugin"]


how safe is this?

I guess it should be safe since if the code reaches this then the plugin entry is there, but it could be written as properties.pop("plugin", None)

It should be safe (the plugin key not existing in the properties dict would mean we have a much bigger problem), but I rewrite it with a list comprehension anyway since that's faster and safer

(And if you meant because of the del, yes that's safe too, as schema creates a new dict each time.)

tigarmo · 2024-07-11T11:54:56Z

craft_parts/plugins/properties.py

@@ -65,4 +81,7 @@ def get_pull_properties(cls) -> list[str]:
    def get_build_properties(cls) -> list[str]:
        """Obtain the list of properties affecting the build stage."""
        properties = cls.schema(by_alias=True).get("properties")
-        return list(properties.keys()) if properties else []
+        if properties:
+            del properties["plugin"]


I guess it should be safe since if the code reaches this then the plugin entry is there, but it could be written as properties.pop("plugin", None)

tigarmo · 2024-07-11T11:56:19Z

craft_parts/plugins/nil_plugin.py


 from overrides import override

 from .base import Plugin
 from .properties import PluginProperties


-class NilPluginProperties(PluginProperties):
+class NilPluginProperties(PluginProperties, frozen=True):


shouldn't this one have _required_fields = ("plugin",)?

Somewhat counter-intuitively, no. The name _required_fields came from it being the required argument in extract_plugin_properties, but in reality it's more like _extra_fields or _fields_that_dont_start_with_the_plugins_name.

Looking at this though, extract_plugin_properties is only actually used here, so I'm thinking about dumping it altogether and moving the logic here anyway.

Ok, I've moved the logic from extract_plugin_properties into PluginProperties.unmarshal() and used the model's properties to determine the fields to use.

tigarmo · 2024-07-11T11:57:47Z

craft_parts/plugins/properties.py

+        validate_assignment=True,
+    )
+
+    plugin: Any = ""


Is it to play nice with the various Literal["x"] overrides in subclasses?

I missed some changes when I was playing with PluginProperties. The plugin name will always be a string.

Moves the logic from extract_plugin_properties directly into PluginProperties.unmarshal()

mr-cal

Nice refactor!

lengau force-pushed the work/CRAFT-2913/2-refactor-plugin-models branch from 27af4e5 to 8e88178 Compare July 10, 2024 15:56

lengau force-pushed the work/CRAFT-2913/2-refactor-plugin-models branch from 8e88178 to edd76ff Compare July 10, 2024 19:15

lengau changed the title ~~chore!: remove PluginModel class~~ chore!: restructure plugin models Jul 10, 2024

lengau force-pushed the work/CRAFT-2913/2-refactor-plugin-models branch 2 times, most recently from ff77dd9 to fcc9096 Compare July 10, 2024 19:41

lengau force-pushed the work/CRAFT-2913/2-refactor-plugin-models branch from fcc9096 to 2d2e216 Compare July 10, 2024 20:31

lengau requested review from tigarmo, mr-cal and cmatsuoka July 10, 2024 20:44

fix: fix integration tests

1899868

lengau force-pushed the work/CRAFT-2913/2-refactor-plugin-models branch from bded288 to 1899868 Compare July 10, 2024 20:56

lengau marked this pull request as ready for review July 10, 2024 20:56

lengau mentioned this pull request Jul 10, 2024

feat: add pydantic models for all sources #776

Merged

1 task

fix(lint): remove some unnecessary specialness

3d9308d

lengau force-pushed the work/CRAFT-2913/2-refactor-plugin-models branch from 0ba2c27 to 3d9308d Compare July 10, 2024 22:41

sergiusens reviewed Jul 11, 2024

View reviewed changes

tigarmo reviewed Jul 11, 2024

View reviewed changes

lengau added 3 commits July 12, 2024 10:37

fix: pr comment

86d8213

I missed some changes when I was playing with PluginProperties. The plugin name will always be a string.

fix: make get_build_properties faster and safer

6042902

feat!(plugins): remove extract_plugin_properties

95e5fa6

Moves the logic from extract_plugin_properties directly into PluginProperties.unmarshal()

lengau force-pushed the work/CRAFT-2913/2-refactor-plugin-models branch from b168041 to 95e5fa6 Compare July 12, 2024 15:16

lengau added 2 commits July 12, 2024 11:19

style(lint): cleanup imports and cast

6b26111

chore(comment): add comment clarifying code

5dc9145

lengau requested review from sergiusens and tigarmo July 12, 2024 15:22

tigarmo approved these changes Jul 12, 2024

View reviewed changes

mr-cal approved these changes Jul 12, 2024

View reviewed changes

lengau added 2 commits July 12, 2024 11:50

fix: convert pydantic validation errors

48363d3

chore(lint): add JSON to spelling dictionary.

c553609

lengau merged commit 029f1ad into feature/2.0 Jul 12, 2024
8 checks passed

lengau deleted the work/CRAFT-2913/2-refactor-plugin-models branch July 12, 2024 16:09

lengau mentioned this pull request Jul 12, 2024

chore: pydantic 2 related cleanup #778

Merged

1 task

lengau linked an issue Jul 23, 2024 that may be closed by this pull request

Migrate to Pydantic 2.x #728

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore!: restructure plugin models #775

chore!: restructure plugin models #775

lengau commented Jul 10, 2024 •

edited

Loading

sergiusens Jul 11, 2024

tigarmo Jul 11, 2024

lengau Jul 12, 2024

sergiusens Jul 11, 2024

tigarmo Jul 11, 2024

lengau Jul 12, 2024

lengau Jul 12, 2024

tigarmo Jul 11, 2024

tigarmo Jul 11, 2024

lengau Jul 12, 2024

lengau Jul 12, 2024

tigarmo Jul 11, 2024

mr-cal left a comment

chore!: restructure plugin models #775

chore!: restructure plugin models #775

Conversation

lengau commented Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-cal left a comment

Choose a reason for hiding this comment

lengau commented Jul 10, 2024 •

edited

Loading