Skip to content
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

Rename the ruff_vendored crate to red_knot_vendored #13586

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

Following #13436, @carljm and I are no longer getting automated review requests for typeshed-sync PRs: @dhruvmanila had to manually request my review on #13578. That's because our CODEOWNERs file only has us as "owning" paths that have red_knot in them:

ruff/.github/CODEOWNERS

Lines 19 to 21 in 2a36b47

# red-knot
/crates/red_knot* @carljm @MichaReiser @AlexWaygood
/crates/ruff_db/ @carljm @MichaReiser @AlexWaygood

We could just add another line to CODEOWNERS, like we already have for ruff_db, but renaming the crate has other advantages too:

  • This crate really is red-knot-specific. If the Ruff linter or formatter decides it needs to vendor some other files in the future, it won't use this crate to do it, since this crate packages all of typeshed into a zip at build time, which is entirely unnecessary for the linter and formatter
  • It's pretty minor, but it's nice to have all the red-knot-related crates next to each other in the folder list when I'm scrolling through them in my editor.

Test Plan

  • cargo test
  • Grepped the source code to check there weren't any remaining references to ruff_vendored anywhere.

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Oct 1, 2024
Copy link
Contributor

github-actions bot commented Oct 1, 2024

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.

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. Unrelated to this PR, do we need the vendor directory inside the red_knot_vendored crate or should we move the typeshed at the top-level? The double "vendor" was confusing at first but it's fine regardless.

@AlexWaygood
Copy link
Member Author

Unrelated to this PR, do we need the vendor directory inside the red_knot_vendored crate or should we move the typeshed at the top-level? The double "vendor" was confusing at first but it's fine regardless.

Yeah, I suppose having the vendor/ directory made more sense when these vendored files were part of a bigger crate like red_knot_module_resolver or red_knot_python_semantic; in those situations, it was useful to separate the "vendored data" from the actual Rust code. But now that they're in their own crate, there's very little actual Rust code 😆

I feel like I still weakly prefer having them in a subdirectory for that reason, but definitely don't have a strong opinion

@MichaReiser
Copy link
Member

This crate really is red-knot-specific. If the Ruff linter or formatter decides it needs to vendor some other files in the future, it won't use this crate to do it, since this crate packages all of typeshed into a zip at build time, which is entirely unnecessary for the linter and formatter

I don't think I agree with this. Ruff analyze might need the same vendored files if it wants to detect standard library or third party dependencies.

I used this name because ruff will use this crate long term. Although that can be said for an red knot module.

TLDR: I don't mind this change.

@AlexWaygood
Copy link
Member Author

Ruff analyze might need the same vendored files if it wants to detect standard library or third party dependencies.

Well, ruff_graph already depends on the red_knot_python_semantic crate, so having another red_knot crate in its dependencies feels like it doesn't hurt that much ;) And the API this crate exposes isn't really usable right now unless you also depend on red_knot_python_semantic.

I feel like we can consider renaming them again when Ruff and red-knot truly merge, but for now it feels very much like part of the red-knot project to me. And renaming is fairly cheap!

@AlexWaygood AlexWaygood merged commit 8232467 into main Oct 1, 2024
20 checks passed
@AlexWaygood AlexWaygood deleted the alex/rename-ruff-vendored branch October 1, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants