[MAINTENANCE] Improve experience around expectation deletion with Cloud-backed suites #10662
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## develop #10662 +/- ##
===========================================
- Coverage 80.47% 80.47% -0.01%
===========================================
Files 462 462
Lines 40107 40106 -1
===========================================
- Hits 32278 32277 -1
Misses 7829 7829
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| @@ -145,19 +145,13 @@ def add_expectation(self, expectation: _TExpectation) -> _TExpectation: | |||
| ) | |||
| should_save_expectation = self._has_been_saved() | |||
| expectation_is_unique = all( | |||
| expectation.configuration != existing_expectation.configuration | |||
| for existing_expectation in self.expectations | |||
| expectation != existing_expectation for existing_expectation in self.expectations | |||
There was a problem hiding this comment.
Equality checks should utilize Expectation.__eq__ rather than configuration
| try: | ||
| expectation = self._store.add_expectation(suite=self, expectation=expectation) | ||
| self.expectations[-1].id = expectation.id | ||
| except Exception as exc: | ||
| self.expectations.pop() | ||
| raise exc # noqa: TRY201 |
There was a problem hiding this comment.
We can just wait to append until we succeed with persistence - also, the id is already attached to the input expectation
| @@ -368,7 +368,7 @@ def __eq__(self, other: object) -> bool: | |||
|
|
|||
| # rendered_content is derived from the rest of the expectation, and can/should | |||
| # be excluded from equality checks | |||
| exclude: set[str] = {"rendered_content"} | |||
| exclude: set[str] = {"rendered_content", "id"} | |||
There was a problem hiding this comment.
Legacy equality checks (i.e. using configuration) exclude id
There was a problem hiding this comment.
Do we want to maintain that? Does still comparing the id break anything? Would it be a bug if the ids don't match? If we need this logic, maybe add it to the comment above about rendered_content?
There was a problem hiding this comment.
I don't love it but if we look at the script I mentioned in Slack, we get different behavior based on this:
suite = context.suites.add(gx.ExpectationSuite("chetan-test-2024-11-13-v21"))
for i in range(10):
e = gxe.ExpectColumnValuesToBeBetween(column="age", min_value=0, max_value=100)
suite.add_expectation(e)
print(len(suite.expectations)) # How many should we see?We historically have gotten 1 from this but making id part of the __eq__ check gets us 10.
| ) | ||
| def test_expectation_equality_with_id(self, id_a: str | None, id_b: str | None): |
…_expectations into b/core-633/expectation_deletion
| except Exception as exc: | ||
| self.expectations.pop() | ||
| raise exc # noqa: TRY201 | ||
| expectation = self._store.add_expectation(suite=self, expectation=expectation) |
There was a problem hiding this comment.
well this seems much more clear!
| @@ -218,13 +212,15 @@ def _process_expectation( | |||
| @public_api | |||
| def delete_expectation(self, expectation: Expectation) -> Expectation: | |||
| """Delete an Expectation from the collection. | |||
| The input Expectation must be in the suite and referenced by index. | |||
There was a problem hiding this comment.
I'm going to throw a blocker on here. The second part of this seems wrong. How could it matter how you retrieve the expectation?
There was a problem hiding this comment.
Hmm this is fair - I want to encourage users to delete by using suite.expectations[idx] instead of gxe.ExpectWhateverWhatever. I think the example here is good enough.
There was a problem hiding this comment.
Yeah, the example is fine with me. FWIW I'd personally probs find it by like next(e for e in suite.expectations where isinstance(e, MyExpectation))
| @@ -368,7 +368,7 @@ def __eq__(self, other: object) -> bool: | |||
|
|
|||
| # rendered_content is derived from the rest of the expectation, and can/should | |||
| # be excluded from equality checks | |||
| exclude: set[str] = {"rendered_content"} | |||
| exclude: set[str] = {"rendered_content", "id"} | |||
There was a problem hiding this comment.
Do we want to maintain that? Does still comparing the id break anything? Would it be a bug if the ids don't match? If we need this logic, maybe add it to the comment above about rendered_content?
| def _determine_if_expectation_is_unique(self, expectation: Expectation) -> bool: | ||
| # Expectation is deemed unique if it is not already in the suite | ||
| # We do not consider the id of the expectation in this check | ||
| expectation_copies = copy.deepcopy(self.expectations) |
There was a problem hiding this comment.
I'm guessing we also want to account for rendered_content, right?
Also does this get expensive when we have rendered content? If we want to save ourselves the deep copy (and mutation), you could do something like model1.dict(exclude={"id"}) == model2.dict(exclude={"id"}) in a loop, or that's what chatgpt tells me at least 😄
| pytest.param(None, None, id="both_none"), | ||
| pytest.param({}, None, id="both_falsy"), | ||
| pytest.param({"author": "Bob Dylan"}, None, id="missing_meta"), | ||
| pytest.param({"author": "Bob Dylan"}, {"author": "John Lennon"}, id="different_meta"), |
There was a problem hiding this comment.
Potentially a huge blocker for me: did these guys actually write books?
| @@ -368,18 +368,12 @@ def __eq__(self, other: object) -> bool: | |||
|
|
|||
| # rendered_content is derived from the rest of the expectation, and can/should | |||
| # be excluded from equality checks | |||
| exclude: set[str] = {"rendered_content"} | |||
| # notes and meta are simple metadata and should be excluded from equality checks | |||
| exclude: set[str] = {"rendered_content", "notes", "meta"} | |||
There was a problem hiding this comment.
Should we really exclude these for equality? Why not just handle these kind of weird situations in the helper method above?
| remaining_expectations = [ | ||
| exp for exp in self.expectations if exp.configuration != expectation.configuration | ||
| ] | ||
| remaining_expectations = [exp for exp in self.expectations if exp != expectation] |
There was a problem hiding this comment.
Should this be using the same kind of logic as in _determine_if_expectation_is_unique wrt ignoring ids?
| expectation: The expectation to check for. | ||
|
|
||
| Returns: | ||
| A tuple of a boolean indicating whether the expectation is already in the suite |
There was a problem hiding this comment.
Non-blocking: the method return type feels like a bit of a smell, and designed for a very specific use case. Would it be simpler if we took in 2 expectations, and just returned a bool, then let delete_expectation be responsible for looping?
Don't feel like you need to over-index on this - we've already had a ton of back and forth, and I appreciate your patience!!
A few relevant changes here:
.configuration)__eq__has specific logic around rendered content and other fields that could go awrynotesandmetashould be excluded from__eq__; they are too volatile due to misc updates from Cloud and represent "metadata" as opposed to actual state in my opinionidwhen determining uniqueness withsuite.add_expectation()(users can very easily add duplicates without realizing)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!