-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
Test dir of pandas.api.* #52826
Test dir of pandas.api.* #52826
Conversation
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.
Thanks for the PR! Looks good - some minor requests.
pandas/tests/api/test_api.py
Outdated
@@ -17,10 +23,12 @@ def check(self, namespace, expected, ignored=None): | |||
result = sorted( | |||
f for f in dir(namespace) if not f.startswith("__") and f != "annotations" | |||
) | |||
|
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.
Can you remove this added line. I think the code is fine either way (someone could argue to remove the blank lines with the justification that they unnecessarily use up vertical space), and in such cases it's best to leave as-is.
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.
Absolutely, thank you for the review. Implemented all the changes.
pandas/tests/api/test_api.py
Outdated
if ignored is not None: | ||
result = sorted(set(result) - set(ignored)) | ||
|
||
expected = sorted(expected) | ||
|
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.
Same
pandas/tests/api/test_api.py
Outdated
@@ -237,8 +245,14 @@ def test_depr(self): | |||
|
|||
|
|||
class TestApi(Base): | |||
allowed = ["types", "extensions", "indexers", "interchange", "typing"] | |||
allowed_typing = [ | |||
allowed_api_dirs: list[str] = [ |
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.
By-and-large, we do not type-hint test code. I think we should keep the status-quo here unless an explicit decision is made to add type-hints. Can you remove throughout.
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.
lgtm
Thanks @SecretLake! |
@rhshadrach Could you please review this PR?