Skip to content

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

Merged
merged 1 commit into from
Jun 25, 2025
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jun 25, 2025

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

@MichaReiser MichaReiser added the internal An internal refactor or improvement label Jun 25, 2025
@MichaReiser MichaReiser force-pushed the micha/delete-ruff-resolver branch from 1d121a1 to 8409b52 Compare June 25, 2025 06:29
@MichaReiser MichaReiser requested a review from AlexWaygood June 25, 2025 06:29
Copy link
Contributor

mypy_primer results

No ecosystem changes detected ✅

Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@AlexWaygood AlexWaygood changed the title Delete the ruff_python_resovler crate Delete the ruff_python_resolver crate Jun 25, 2025
@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 25, 2025

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 😛

@MichaReiser
Copy link
Member Author

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"

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

@AlexWaygood
Copy link
Member

Right, but I'm terrible at typescript and prefer reading Charlie's rust port 😆

@MichaReiser
Copy link
Member Author

MichaReiser commented Jun 25, 2025

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.

@AlexWaygood
Copy link
Member

It sounds like you feel more strongly than I do — don't let me block you!

it has confused contributors because they thought they could build on top of it.

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

@MichaReiser
Copy link
Member Author

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.

@MichaReiser MichaReiser merged commit c1fed55 into main Jun 25, 2025
36 checks passed
@MichaReiser MichaReiser deleted the micha/delete-ruff-resolver branch June 25, 2025 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants