-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Filter private symbols from stubs if they are internal types #19121
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
|
This comment was marked as outdated.
This comment was marked as outdated.
public_explicit_type_alias: TypeAlias = Literal[1] | ||
_private_explicit_type_alias: TypeAlias = Literal[1] |
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.
@AlexWaygood why "A private type alias that is explicitly annotated with typing.TypeAlias"? It looks like the behavior doesn't change either way.
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.e., the naive alternative passes
diff --git a/crates/ty_ide/src/completion.rs b/crates/ty_ide/src/completion.rs
index 9a6190cb5..0ee759051 100644
--- a/crates/ty_ide/src/completion.rs
+++ b/crates/ty_ide/src/completion.rs
@@ -536,6 +536,9 @@ _private_type_var_tuple = TypeVarTuple("_private_type_var_tuple")
public_explicit_type_alias: TypeAlias = Literal[1]
_private_explicit_type_alias: TypeAlias = Literal[1]
+public_implicit_type_alias = Literal[1]
+_private_implicit_type_alias = Literal[1]
+
class PublicProtocol(Protocol):
def method(self) -> None: ...
@@ -558,6 +561,8 @@ class _PrivateProtocol(Protocol):
test.assert_completions_do_not_include("_private_type_var_tuple");
test.assert_completions_include("public_explicit_type_alias");
test.assert_completions_include("_private_explicit_type_alias");
+ test.assert_completions_include("public_implicit_type_alias");
+ test.assert_completions_include("_private_implicit_type_alias");
test.assert_completions_include("PublicProtocol");
test.assert_completions_do_not_include("_PrivateProtocol");
}
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.
oh whoops -- for some reason I thought that my patch excluded explicitly declared type aliases with private names. But you're quite right -- it doesn't do that. We'd probably need to add a new variant to the DynamicType
enum to distinguish type-alias Todo
types from other Todo
types in order to get that working. (Shouldn't be too hard to do as a followup, I can pick it up.)
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'll leave it to you then :)
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 picked this up in #19282)
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.
nice!
/// Unlike [`private_symbols_in_stub`], this test doesn't use a `.pyi` file so all of the names | ||
/// are visible. | ||
#[test] | ||
fn private_symbols_in_module() { |
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.
It could be nice to avoid the repetition here, but I don't love optimizing tests for that early. Thoughts?
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.
We could also just test a few of the "do not include" cases here.
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.
For just two tests, I think this is fine to be honest. (And it seems like two cases is what we want.)
74af9ee
to
65f7338
Compare
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
This implements filtering of private symbols from stub files based on type information as discussed in #19102. It extends the previous implementation to apply to all stub files, instead of just the
builtins
module, and uses type information to retain private names that are may be relevant at runtime.