-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Delete the ruff_python_resolver
crate
#18933
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
1d121a1
to
8409b52
Compare
|
|
ruff_python_resovler
crateruff_python_resolver
crate
I still feel like this will be a useful reference for some of the more advanced features pyright's resolver has that ty's doesn't yet have, but which we want to add in the future. For example, pyright's resolver has the ability to resolve a module to a C extension, and therefore records the fact that the module does actually exist at runtime, pyright just has no types for it. That allows pyright to issue different diagnostics for "the module isn't installed" vs "the module is installed but you'll need to install some stubs before I can do type inference properly". I guess this crate will always be in Ruff's git history if you really want to remove it now, though 😛 |
If that's the only use, then I suggest that we look at Pyright's module resolver implementation (which was the inspiration for our implementation). https://github.com/microsoft/pyright/blob/e5ecd9e32f54db97222a982db26fb182c34125d8/packages/pyright-internal/src/analyzer/importResolver.ts#L716 |
Right, but I'm terrible at typescript and prefer reading Charlie's rust port 😆 |
I mean, the port is type script written in Rust (and it's also not much TypeScript, it's mostly c-like code). Anyway, I don't think these are strong enough reasons to keep the code lingering, especially considering that it has confused contributors because they thought they could build on top of it. |
It sounds like you feel more strongly than I do — don't let me block you!
This is a great motivation, and one I haven't got much insight into, because I haven't been engaging with Ruff contributors to nearly the same extent you have over the past few months |
Thank you. It doesn't come up often but every now and then. There was another instance today (#9103 (comment)) which is why I opened this PR, very well knowing that we decided not to merge similar PRs for the reasons you mentioned before. |
Summary
We now have a working module resolver in
ty_python_semantic
.ruff_python_resolver
is unused, has very little test coverage, and gives the wrong impression that this could be used in ruff.We didn't delete this in the past because we wanted it as a reference for ty's module resolver. I think we're now at a point where this is no longer necessary (ty's module resolver is further along). We can also always take a look at pyright's module resolver, from which this implementation was inspired from.
Test Plan
cargo build