-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TYP: use __all__ to signal public API to type checkers #43695
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
Conversation
twoertwein
commented
Sep 22, 2021
•
edited
Loading
edited
- closes TYP: Redundant imports to define the public API #43664
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
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.
No, but once we add a py.typed file we could use pyright to make sure that a list of intended to be public functions/attributes are picked up as public by type checkers. |
pandas/__init__.py
Outdated
import pandas.testing | ||
import pandas.arrays | ||
from pandas import testing | ||
from pandas import arrays |
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.
Is there a reason why api
, testing
, and arrays
was not imported with "from pandas import"? I changed it to from pandas import
to include them in __all__
.
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.
these should be equivalent (ex the name space visiblity).
note that we may not be fully testing this (see in pandas/tests/api/test_api.py)
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 added a test for __all__
and found in that process a few un-exported modules.
assert not extraneous | ||
|
||
missing = expected - actual | ||
assert not missing |
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.
assert actual == expected
would work, but it doesn't create a nice log in pytest.
I think the modules I added in the last commit are not meant to be public (compat, core, io, ...)? @simonjayhawkins If the above modules are not meant to be public, there probably needs to be a discussion which additional classes should be exported in def mean(grouper: pd.core.groupby.DataFrameGroupBy) -> pd.DataFrame:
return group.mean() without using "private" APIs (pd.core.groupby.DataFrameGroupBy). |
none of those are documented as public we have an issue about deprecating and making these private but hasn't gotten done don't add them explicitly to any exports |
some of these apis should be exported in pd.api as well (eg DataframeGroupby for example) |
So the only public "top-level sub-packages" are: "api", "arrays", "options", "test", "testing"? They are all imported in |
Reading through the documentation, the following should be listed as public in |
I was trying to expose only meant-to-be-public sub-modules (public = listed in the documentation) in Here is an example how I would # import modules that have public classes/functions
from pandas.io import (
formats,
json,
stata,
)
# and mark only those modules as public
__all__ = ["formats", "json", "stata"] I think many people assume that everything that doesn't start with an underscore is public. |
can you rebase |
…all__ (everything else is technically private)
https://github.com/pandas-dev/pandas/pull/43695/checks?check_run_id=3734385197 hmm maybe need to be more explict in the docs |
I think there are two options:
Which one is preferred? |
i don't think we should add the deprecated modules to all |
thanks @twoertwein very nice! |