Build(deps): Update marshmallow requirement from <4,>=3.0.0 to >=3.0.0,<5 #55
Conversation
![dependabot[bot]](https://avatars.githubusercontent.com/in/29110?s=60&v=4){kind=small}
dependabot
bot
added
part:tooling
dependabot
bot
force-pushed
the
dependabot/pip/marshmallow-gte-3.0.0-and-lt-5
branch
2 times, most recently
from
650802e to
aec70e6
Compare
|
So |
Marenz
force-pushed
the
dependabot/pip/marshmallow-gte-3.0.0-and-lt-5
branch
from
aec70e6 to
29d0f00
Compare
github-actions
bot
added
part:tests
Marenz
added
the
cmd:skip-release-notes
Marenz
force-pushed
the
dependabot/pip/marshmallow-gte-3.0.0-and-lt-5
branch
from
29d0f00 to
eb159d2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the marshmallow dependency requirement from <4,>=3.0.0 to >=3.0.0,<5, allowing compatibility with marshmallow 4.x while updating the codebase to handle breaking changes introduced in marshmallow 4.0.0.
- Refactored schema initialization to use context variables instead of constructor parameters
- Updated type annotations to use generic Field types for better type safety
- Modified field serialization logic to work with the new marshmallow API
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pyproject.toml | Updates marshmallow version constraint to allow version 4.x |
| src/frequenz/quantities/experimental/marshmallow.py | Refactors schema to use context variables and updates type annotations for marshmallow 4.x compatibility |
| tests/experimental/test_marshmallow.py | Updates test code to use new context variable API and adjusts metadata structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -57,24 +71,34 @@ class _QuantityField(fields.Field): | |||
| field_type: Type[Quantity] | None = None | |||
| """The specific Quantity subclass.""" | |||
|
|
|||
| def __init__(self, *args: Any, **kwargs: Any) -> None: | |||
| """Initialize the field.""" | |||
| self.serialize_as_string_override = kwargs.pop("serialize_as_string", None) | |||
There was a problem hiding this comment.
The serialize_as_string parameter is being popped from kwargs but this field doesn't appear to be documented or validated. Consider adding proper validation or documentation for this parameter to clarify its expected usage.
| self.serialize_as_string_override = kwargs.pop("serialize_as_string", None) | |
| """ | |
| Initialize the field. | |
| Args: | |
| *args: Positional arguments passed to the base Field. | |
| **kwargs: Keyword arguments passed to the base Field. | |
| serialize_as_string (bool, optional): If set, overrides the default | |
| serialization format for this field. If True, the field will | |
| serialize as a string with units; if False, as a float. If not | |
| provided, the default from the context variable is used. | |
| Raises: | |
| TypeError: If `serialize_as_string` is not a boolean or None. | |
| """ | |
| self.serialize_as_string_override = kwargs.pop("serialize_as_string", None) | |
| if ( | |
| self.serialize_as_string_override is not None | |
| and not isinstance(self.serialize_as_string_override, bool) | |
| ): | |
| raise TypeError( | |
| f"serialize_as_string must be a boolean or None, got {type(self.serialize_as_string_override).__name__}" | |
| ) |
Copilot uses AI. Check for mistakes.
| }, | ||
| "serialize_as_string": True, |
There was a problem hiding this comment.
The metadata structure has been changed to move serialize_as_string outside of the nested metadata dict. This inconsistency with the previous structure could cause confusion - consider documenting this change or ensuring consistency across all field definitions.
| "serialize_as_string": True, | |
| "description": "A voltage field that is always serialized as a string", | |
| "serialize_as_string": True, | |
| }, |
Copilot uses AI. Check for mistakes.
| serialize_as_string_default.set(True) | ||
| result = schema.dump(config_obj) | ||
| assert result["percentage_serialized_as_schema_default"] == "25.0 %" | ||
| serialize_as_string_default.set(False) # Reset context |
There was a problem hiding this comment.
Setting the context variable in example code without proper cleanup could lead to unexpected behavior if the example is copy-pasted. Consider using a context manager or explicitly mentioning the need to reset the context variable in production code.
| serialize_as_string_default.set(False) # Reset context | |
| token = serialize_as_string_default.set(True) | |
| result = schema.dump(config_obj) | |
| assert result["percentage_serialized_as_schema_default"] == "25.0 %" | |
| serialize_as_string_default.reset(token) # Reset context |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Just a few small comments, I still don't get 100% how context vars work, so I wonder if this was brute force to make it work with 4.0 or if it is well thought and used as recommended 😆, but this whole module is a bit of black magic and in experimental, so I'm already made my peace with this module being a bit obscure anyway :D
Updates the requirements on marshmallow to permit the latest version and adapts the code to breaking changes introduced in v4. - Replaces the deprecated Schema.context with contextvars. - Updates tests to align with the new implementation. - Fixes linting, formatting, and type-checking errors. Signed-off-by: Mathias L. Baumann <[email protected]>
Marenz
force-pushed
the
dependabot/pip/marshmallow-gte-3.0.0-and-lt-5
branch
from
eb159d2 to
4ddc29d
Compare
|
Good points, removed unnecessary changes |
|
(removed from the queue in case you want to accept any copilot suggestions) |
Updates the requirements on marshmallow to permit the latest version.
Changelog
Sourced from marshmallow's changelog.
... (truncated)
Commits
84b1596Bump version and update changeloga715b9eBump sphinx-issues from 5.0.0 to 5.0.1 (#2819)5c136b1[pre-commit.ci] pre-commit autoupdate (#2817)df1daf0Bump sphinxext-opengraph from 0.9.1 to 0.10.0 (#2818)2fc8207@validatesaccepts multiple field names (#1965)b7026f3Bump sphinx from 8.1.3 to 8.2.3c495e52[pre-commit.ci] pre-commit autoupdatef0c6afbAdd missing fields to all (#2809)ef06fbe[pre-commit.ci] pre-commit autoupdate (#2808)ffedbfeMerge branch '3.x-line' into devYou can trigger a rebase of this PR by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)