-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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] ExpectationSuite
API cleanup
#9875
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
assert all( | ||
mine.isEquivalentTo(theirs) | ||
for (mine, theirs) in zip( | ||
single_expectation_suite.expectation_configurations, | ||
different_suite.expectation_configurations, | ||
) | ||
) |
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.
Took the guts of the isEquivalentTo
method and used it for the assertion here
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9875 +/- ##
===========================================
- Coverage 78.63% 78.61% -0.03%
===========================================
Files 484 484
Lines 42537 42394 -143
===========================================
- Hits 33450 33329 -121
+ Misses 9087 9065 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
return removed_expectations | ||
|
||
def find_expectation_indexes( | ||
def _find_expectation_indexes( |
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 is used by other methods so it has to stay around but I don't think it needs to be public
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.
I feel like we're misusing this. This method isn't private; it's deprecated. And it's already not in the public api. typing-extensions
has a deprecated
decorator. Thoughts on just using that?
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.
I suppose I care less about the distinction if the only external use case is in a test
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.
Just to close the loop on this, this method is only ever used within this class (at least after the other deletions in this file).
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.
I want to at least discuss not using the psuedo-private _
prefix for methods that are used outside of the class.
self.meta["citations"].append(citation) | ||
|
||
# noinspection PyPep8Naming | ||
def isEquivalentTo(self, other): |
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.
I thought this one was too hard to pull out? Awesome if we can do it now!
return removed_expectations | ||
|
||
def find_expectation_indexes( | ||
def _find_expectation_indexes( |
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.
I feel like we're misusing this. This method isn't private; it's deprecated. And it's already not in the public api. typing-extensions
has a deprecated
decorator. Thoughts on just using that?
Delete any extraneous methods we don't need (citations, filtering, etc). Also, make any methods private if we can.
invoke lint
(usesruff format
+ruff check
)For more information about contributing, see Contribute.
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!