(WIP) CI: Check that private functions are not used across modules #32942
Conversation
ShaharNaveh
force-pushed
the
CI-check-for-no-internal-functions-across-modules
branch
from
7ec9851 to
d9f3928
Compare
| if not (isinstance(node, ast.Import) or isinstance(node, ast.ImportFrom)): | ||
| continue | ||
|
|
||
| if any(mod.name.split(".")[-1].startswith("_") for mod in node.names): | ||
| yield (node.lineno, "Use of private function across modules found.") |
There was a problem hiding this comment.
The logic behind this is as follow:
-
If the node is not
importorfrom foo import barwe don't care here, so continue to loop. -
if the imported function name starts with an underscore(
_), it will yield the line number of the import statement.
There was a problem hiding this comment.
can you add the function name to the message
There was a problem hiding this comment.
Sure!
Since 4786cf7 the private function imported name is included in the message.
|
neat, thanks for taking the lead on this. Looks like we've got plenty of violations here. let's stick a pin in this and try to whittle those down over the coming weeks, then revisit |
@jbrockmendel Should I open a master issue on this? |
You could open a STY issue, sure. The other (harder) thing that would go with this would be checking more generally for Or could do a round-up for STY issues, since I know there are a bunch. |
|
@MomIsBestFriend how hard would it be to make a special case to allow _shared_docs and similar? |
@jbrockmendel Very easy, can you please provide me a list of names you'd like to have an exception for? |
…nternal-functions-across-modules
Subject to change, let's start with ["_merge_doc", "_shared_docs", "_extension_array_shared_docs", "_index_shared_docs"] |
|
Note: ATM I'm only checking |
|
After the changes in #33216 are accepted, the whole |
|
@MomIsBestFriend can you re-push? re-running the failed GH Actions check isnt working for some reason |
…nternal-functions-across-modules
|
rebased @jbrockmendel |
|
@jbrockmendel As I was going through the code I noticed that for example at Line 415 in 213822a
Line 223 in 24827e5 and in pandas/pandas/core/internals/blocks.py Line 379 in ef1cac3 ATM the check is doing "lazy" check, as it only checking for imports of It will not detect things like import foo as foolib
value = foolib._private_function("baz")Do you want the existing check to validate the case above as well? |
That's going to be the next step, but its going to have a lot of violations |
…nternal-functions-across-modules
…nternal-functions-across-modules
|
Let's tentatively add to the ignored list all of the remaining docstring template stuff and pickle-constructor stuff: _new_Index, We should make the _np_under_17 public, will take another look at whats left. |
…nternal-functions-across-modules
…-check-for-no-internal-functions-across-modules
|
just rebased and pushed |
|
@MomIsBestFriend brainstorming: how hard would it be to check for tests that just silently |
…-check-for-no-internal-functions-across-modules
…-check-for-no-internal-functions-across-modules
|
nice idea. pls comment / open a new PR if able to continue. |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffcc @jbrockmendel