Add marshmallow helpers #24

Marenz · 2024-10-22T10:33:00Z

No description provided.

src/frequenz/quantities/_marshmallow.py

llucax

Awesome!

You still need to expose the new classes, right? I would just make the marshmallow module public and that's it, so we don't mix the extension with the actual quantities code.

pyproject.toml

src/frequenz/quantities/_marshmallow.py

Marenz · 2024-10-23T10:31:47Z

You still need to expose the new classes, right?

I mean, not really, it is enough to use the QuantitySchema, the rest works through mapping in the schema

llucax · 2024-10-23T10:42:29Z

But you still need to expose at least QuantitySchema! 😆

llucax · 2024-10-23T10:43:18Z

BTW, I would expose the fields too, just in case other people want to make some advanced use and do their own mapping or something.

llucax

It would also be nice to add some tests for this.

src/frequenz/quantities/_marshmallow.py

Marenz · 2024-10-28T09:03:15Z

But you still need to expose at least QuantitySchema!

I have! I just forgot to add that to the commit!

Marenz · 2024-10-28T19:03:19Z

Updated

Exposed each quantity field class for the power users (QUANTITY_FIELD_CLASSES)
Added support for string based de/serialization
Added support for default/field based string/float serialization parameters
Added tests
Added support for all quantities
Renamed to "marshmallow" for direct import
Updated docs

llucax · 2024-10-29T11:33:34Z

Added support for string based de/serialization

Haha, so much for a different PR...

llucax

Some more comments, but really cool stuff.

llucax · 2024-10-29T11:37:33Z

src/frequenz/quantities/marshmallow.py

+_QUANTITY_SUBCLASSES = [
+    Current,
+    Energy,
+    Frequency,
+    Percentage,
+    Power,
+    Temperature,
+    Voltage,
+]
+
+
+def _create_quantity_field_class(
+    quantity_subclass: Type[Quantity],
+) -> Type[fields.Field]:
+    """Dynamically create a QuantityField subclass for a given Quantity subclass."""
+    class_name = f"{quantity_subclass.__name__}Field"
+
+    field_class: Type[fields.Field] = type(
+        class_name,
+        (_QuantityField,),
+        {
+            "__init__": lambda self, **kwargs: super(field_class, self).__init__(
+                field_type=quantity_subclass, **kwargs
+            ),
+            "__module__": __name__,
+        },
+    )
+
+    return field_class


Oh, I would much rather have a LLM generating this once than generating type dynamically. For example the documentation won't be properly generated:

I get the sentiment, but.. I am pretty sure no-one really needs to use those classes directly

Like, if we want documentation for this, i would just refer people to the QuantityField class which in that case we should make public then.

but again, I don't see any use-case why someone wants to access them.

That's a fair point, but it really makes it virtually impossible to discover these classes, and makes reading the docs of the module (which is important because of QuantitySchema) very awkward.

To be honest if we are going to keep this code as it is, then I wouldn't export those classes at all, I think exporting them like this is basically useless and makes reading the docs about the important stuff more difficult.

I agree exporting those classes is useless :P

Though I guess that exported variable here can be used for a users own Schema.TYPE_MAPPING, so there is that.

llucax · 2024-10-29T11:40:26Z

src/frequenz/quantities/marshmallow.py

+class _QuantityField(fields.Field):
+    """Custom field for Quantity objects supporting per-field serialization configuration."""
+
+    def __init__(self, field_type: Type[Quantity], **kwargs: Any) -> None:


I would specify the options explicitly here (like serialize_as_string_default), and only leave the **kwargs for the options that are forwarded to the super() if possible. Otherwise is hard to know which options it really takes in the docs (for the parent, you can always go check the parent class docs).

That seems.. complicated?
For one, that field class and init c'tor is pretty much never used directly.

Why not keep the configuration as it is done here, through the "metadata" key
where we also configure all the other field specific things like validation, key name etc..

Like, I see zero reason to do this one thing differently than all the other things we can configure per-field?

I guess I just don't understand this code very well. I thought this was a base class, but now I see it takes the field_type as argument instead. I don't know, compared to the previous version where there was a class for each field, for me this is extremely hard to read and understand, that's my main complain here 😟

It took me a while as well 😅

This class works for all quantities.
The only thing derived classes need to do is pass their specific quantity type.

Deserialization automatically detects the type (float/str), serialization uses the schema default which can be overridden by a field-specific key.

I still don't understand why, specially now that boilerplate code like this can be automatically generated with a few tabs, we want to keep dynamic unreadable code instead of having some much more obvious as self-explanatory. I see zero advantages of this approach compared to the verbose previous version and a lot of drawbacks.

I still don't understand why

Why what? Why dynamic creation? Because that would be a different comment thread..

I am not strongly against it, I just see zero benefit as noone uses the resulting code directly..
And I doubt it makes things much clearer really.. just lots of noise

From the end user PoV I'm fine with making all the fields "private" and not export them at all if we think nobody will use them. For me one or the other, but the current approach it's just exporting something that's unusable for anyone reading the docs and only adding confusion.

In terms of code, I really don't understand this code and would really prefer not to have it in the repository, again, specially when there is a super clear and easy alternative. There is no need for any black magic here, it should be very simple. The only reason I can see for it is to save a few keystrokes.

That said, I don't feel strong enough to keep investing time on it, I'm fine to merge as is as long as we don't export the fields for now, then we can see in a future PR to improve it to be able to export the fields if there is a need for it.

I see I'm always late with my comments that end up in "I'm fine with merging as is" 😝

src/frequenz/quantities/marshmallow.py

Marenz · 2024-10-29T12:31:19Z

Removed EXCLUDE and updated documentation extensively

tests/test_marshmallow.py

Signed-off-by: Mathias L. Baumann <[email protected]>

Marenz · 2024-10-29T14:23:08Z

Updated: Now explicitly writing out all QuantityField subclasses

Instead of dynamic generation we now generate the whole class so it can be discovered and documented. Signed-off-by: Mathias L. Baumann <[email protected]>

Marenz requested a review from a team as a code owner October 22, 2024 10:33

Marenz requested a review from ela-kotulska-frequenz October 22, 2024 10:33

github-actions bot added part:docs Affects the documentation part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:code Affects the code in general labels Oct 22, 2024

Marenz force-pushed the marsh branch 2 times, most recently from eebed9c to 8e8516b Compare October 22, 2024 10:44

daniel-zullo-frequenz reviewed Oct 22, 2024

View reviewed changes

llucax reviewed Oct 23, 2024

View reviewed changes

pyproject.toml Show resolved Hide resolved

src/frequenz/quantities/_marshmallow.py Outdated Show resolved Hide resolved

src/frequenz/quantities/_marshmallow.py Outdated Show resolved Hide resolved

src/frequenz/quantities/_marshmallow.py Outdated Show resolved Hide resolved

llucax reviewed Oct 23, 2024

View reviewed changes

src/frequenz/quantities/_marshmallow.py Outdated Show resolved Hide resolved

Marenz force-pushed the marsh branch from 8e8516b to 7805e7c Compare October 28, 2024 18:56

github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Oct 28, 2024

Marenz requested a review from llucax October 28, 2024 19:05

Marenz force-pushed the marsh branch 2 times, most recently from 029065b to cbd3e48 Compare October 29, 2024 10:42

llucax reviewed Oct 29, 2024

View reviewed changes

Marenz force-pushed the marsh branch from cbd3e48 to d5a3bc5 Compare October 29, 2024 12:30

daniel-zullo-frequenz reviewed Oct 29, 2024

View reviewed changes

tests/test_marshmallow.py Outdated Show resolved Hide resolved

Add marshmallow helpers

bd3cf08

Signed-off-by: Mathias L. Baumann <[email protected]>

Marenz force-pushed the marsh branch from d5a3bc5 to bd3cf08 Compare October 29, 2024 13:52

Marshmallow: Explictly create subclasses for QuantityFields

91934a2

Instead of dynamic generation we now generate the whole class so it can be discovered and documented. Signed-off-by: Mathias L. Baumann <[email protected]>

Marenz force-pushed the marsh branch from 757cc96 to 91934a2 Compare October 29, 2024 14:24

llucax approved these changes Oct 29, 2024

View reviewed changes

Marenz added this pull request to the merge queue Oct 30, 2024

Merged via the queue into frequenz-floss:v1.x.x with commit 5635738 Oct 30, 2024
14 checks passed

Marenz deleted the marsh branch October 30, 2024 09:20

llucax added this to the v1.0.0 milestone Oct 30, 2024

Add marshmallow helpers #24

Add marshmallow helpers #24

Uh oh!

Conversation

Marenz commented Oct 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Marenz commented Oct 23, 2024

Uh oh!

llucax commented Oct 23, 2024

Uh oh!

llucax commented Oct 23, 2024

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Marenz commented Oct 28, 2024

Uh oh!

Marenz commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llucax commented Oct 29, 2024

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Marenz commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Marenz commented Oct 29, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Marenz commented Oct 28, 2024 •

edited

Loading

Marenz commented Oct 29, 2024 •

edited

Loading