[MAINTENANCE] Fix schema_name deprecation warning#11849
Conversation
✅ Deploy Preview for niobium-lead-7998 canceled.
|
There was a problem hiding this comment.
Pull request overview
This PR adjusts how TableAsset.schema_name deprecation warnings are emitted so existing configs that deserialize schema_name: null don’t trigger warnings, while making the warning message more actionable by including the asset name. It also removes now-unnecessary pytest warning filters and adds targeted tests to lock in the behavior.
Changes:
- Update
TableAsset’sschema_namevalidator to skip warnings forNone/missing values and include the asset name in the warning text. - Add unit tests validating when the deprecation warning should (and should not) fire, and that it includes the asset name.
- Remove pytest
filterwarningsentries that previously maskedschema_namedeprecation warnings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/datasource/fluent/test_sql_datasources.py | Adds tests for schema_name deprecation warning behavior and message content. |
| pyproject.toml | Removes pytest warning filters for deprecated schema_name warnings. |
| great_expectations/datasource/fluent/sql_datasource.py | Changes schema_name deprecation warning logic and message to avoid warning on None and include asset name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| deprecation_warnings = [ | ||
| w | ||
| for w in caught | ||
| if issubclass(w.category, DeprecationWarning) and "schema_name" in str(w.message) |
There was a problem hiding this comment.
This list-comprehension line exceeds the repo's Ruff line-length = 100 and will likely fail the ruff check (E501). Please reformat/wrap the comprehension (or split the predicate) so the line length stays within 100 characters.
| if issubclass(w.category, DeprecationWarning) and "schema_name" in str(w.message) | |
| if ( | |
| issubclass(w.category, DeprecationWarning) | |
| and "schema_name" in str(w.message) | |
| ) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #11849 +/- ##
===========================================
- Coverage 84.74% 84.73% -0.01%
===========================================
Files 471 471
Lines 39160 39161 +1
===========================================
- Hits 33185 33184 -1
- Misses 5975 5977 +2 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tyler-hoffman
changed the title
tyler-hoffman
changed the title
tyler-hoffman
changed the title
| def _schema_name_deprecation_warning( | ||
| cls, v: str | Missing | None, values: dict | ||
| ) -> str | Missing | None: | ||
| if v is MISSING or v is None: |
There was a problem hiding this comment.
If v is None, that means the user passed schema_name=None right? We don't want that, because eventually we will remove schema_name and break them.
There was a problem hiding this comment.
It means that OR - and this is what Don was hitting in the ticket I believe - we hit this when loading data from the server. But I guess you're right that we probably do want to keep warning if users pass it.. maybe I should just move this to make mercury filter out that field
There was a problem hiding this comment.
I see. It sounds like mercury "cleanup" needs to happen. Even if he was using an old asset, I wouldn't expect to see the warning.
| "once:Passing a string to the row_condition parameter is deprecated:DeprecationWarning", | ||
| # Existing cloud configs store "schema_name": null. During deserialization, Pydantic passes | ||
| # None to the validator which triggers the deprecation warning. The TableAsset.dict() override | ||
| # strips schema_name from new serializations, so these will clean up over time. |
There was a problem hiding this comment.
Looks like these got cleaned up from our cloud org! I think cloud-tests was failing and that's why I added this.
Resolves GX-3216.
Summary of changes:
invoke lint(usesruff format+ruff check)For more information about contributing, visit our community resources.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!