-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Workspace discovery #14308
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
Workspace discovery #14308
Conversation
3cd8c12
to
7522b98
Compare
|
82a48ae
to
1fc09bb
Compare
Oh damn you pre-commit. I'm just going to exclude that file over wasting time fighting pre-commit configuration. |
1a6640a
to
35d3969
Compare
I will review this today. |
Oh I'm not requested here -- that's fine actually. Some feedback on the summary:
No strong opinion though it'd be nice to change this in uv if you decide to do something different here.
I would probably advise against doing this... It makes the logic more complex to implement, test, and document, and it deviates from uv without good reason? I would be surprised if this ever comes up. Even the example you gave isn't quite right given that the exclude contains a dash, if I'm understanding correctly ;) |
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.
The workspace discovery looks great, thank you!
My main question about this PR (and most of my comments) are about how we intend to map these workspace settings to a module resolver search paths config (which is one of the most important outputs from workspace discovery.) I think currently in this PR there is a mismatch between workspace discovery and the module resolver's translation to search paths, which means we will do the wrong thing in a lot of cases, but I'm not sure if it's the module resolver or the workspace code that should change, because I don't understand what the intended semantics of SearchPathSettings::workspace_root
are.
My review is mostly focused on these semantic and project structure questions; relying on Charlie's greater relevant experience from uv
for the more filesystem and pyproject oriented stuff.
… identical names (#9094) ## Summary Align uv's workspace discovery with red knots (see astral-sh/ruff#14308 (comment)) * Detect nested workspace inside the current workspace rather than testing if the current workspace is a member of any outer workspace. * Detect packages with identical names. ## Test Plan I added two integration tests. I also back ported the tests to main to verify that both these invalid workspaces aren't catched by uv today. That makes this, technically, a breaking change but I would consider the change a bug fix. --------- Co-authored-by: Charlie Marsh <[email protected]>
9bcdd7d
to
fd81fb1
Compare
Summary
Disocver workspaces and packages by searching for
pyproject.toml
. This PR loads the pyproject.toml files but it disregards all settings other thanknot.workspace
andproject.name
. I plan to add settings support in a separate PR (this one is already chunky).path
's ancestor chain and find the firstpyproject.toml
.pyproject.toml
contains noknot.workspace
table, then keep traversing thepath
's ancestorchain until we find one or reach the root.
package (the first found package without a
knot.workspace
table is a member. If not, createa single package workspace for the closest package.
pyproject.toml
with aknot.workspace
table, then create a single-package workspace.pyproject.toml
, create an ad-hoc workspace forpath
that consists of a single package and uses the default settings.
Differences to uv
I tried to keep the implementation close to uv's behavior. The main differences are:
pyproject.toml
(nor in any of its ancestor) is a valid use case. Red knot then creates a single package workspace with the default settings.pyproject.toml
without aproject
table.Red knot includes a member if it is explicitly listed in members but matches aexclude
glob. E.g.members=["packages/*", "frontend"]
andexclude=["frontend-*]
still includesfrontend
if the directory is an exact match (not a glob-match). I think uv always excludesfrontend
which seems counterintuitivemembers
globs and then removing excluded members. uv does that too, but not for the "current package" where it usesis_excluded_from_workspace
andis_included_in_workspace
. I found using only globs easier. (https://github.com/astral-sh/uv/blob/e47e8fe9656b9f23891f9651f7bbc881f9e2bc7f/crates/uv-workspace/src/workspace.rs#L1133-L1147)CC: @charliermarsh
Test Plan
Added unit tests and basic file watcher tests. We should add more file watcher tests but I struggled coming up with good examples because most test cases require some form of configuration support to make the change observable.