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
[MAINTENANCE] filter RemovedInMarshmallow4
warnings
#6602
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
👇 Click on the image for a new way to code review
Legend |
786ce0d
to
8270629
Compare
def mm_field( | ||
field_type: Type[marshmallow.fields.Field], load_default: Any = mm_missing, **kwargs | ||
) -> marshmallow.fields.Field: | ||
""" | ||
Wrapper around `marshmallow.fields.String` that preserves backwards compatibility on older versions. | ||
""" | ||
_raise_for_deprecated_kwargs(DEPRECATED_FIELD_ARGS, kwargs) | ||
|
||
if MARSHMALLOW_VERSION < MM_LOAD_DEFAULT_DEPRECATED_VERSION: | ||
kwargs["missing"] = load_default | ||
else: | ||
kwargs["load_default"] = load_default | ||
print(kwargs) | ||
return field_type(**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided that it's much simpler (and easier to maintain) to simply filter out the marshmallow4 deprecation warnings. We aren't moving to marshmallow v4
anytime soon (if ever).
@@ -976,7 +982,7 @@ class Meta: | |||
module_name = fields.String( | |||
required=False, | |||
allow_none=True, | |||
load_default="great_expectations.execution_engine", | |||
missing="great_expectations.execution_engine", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we forgot to revert this use of missing
as part of #6271
# We likely won't be updating to `marhsmallow` 4, these errors should be filtered out | ||
"error::marshmallow.warnings.RemovedInMarshmallow4Warning" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will cause the tests to error if this warning is encountered.
RemovedInMarshmallow4Warning
warningsRemovedInMarshmallow4
warnings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this @Kilo59 🙇🏼 I think the change + note is a elegant solution
Resolves #6440, #6130
The deprecations discussed in the above issues are for a
RemovedInMarshmallow4
warning.We are unlikely to upgrade to
marshmallow
v4
for multiple reasons.pydantic
for schema definitions and validation.marshmallow v3
so upgrading tov4
would also break compatibility with them.If a package in our ecosystem does need to upgrade to
marshmallow v4
we can opt to re-vendorize themarshmallow
which would remove the v3 constraint for any dependency resolver.Changes proposed in this pull request:
RemovedInMarshmallow4
warnings from our codebase.Alternatives considered
a. [MAINTENANCE] Update Marshmallow API Signature #6440 (comment)
b. Initial
deprecation_wrappers.py
POCmarshmallow
version.a. [MAINTENANCE] Small clean-up of
Marshmallow
warnings (missing
parameter changed toload_default
as of 3.13) #6213b. [MAINTENANCE] Reverting
marshmallow
version bump #6271