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

Discover workspaces without using them in resolution #3585

Merged
merged 1 commit into from
May 21, 2024

Conversation

konstin
Copy link
Member

@konstin konstin commented May 14, 2024

Add minimal support for workspace discovery, only used for determining paths in the bluejay commands.

We can now discover the workspace structure, namely that the pyproject.toml of a package belongs to a workspace pyproject.toml with members and exclusion. The globbing logic is inspired by cargo. We don't resolve workspace = true metadata declarations yet.

@konstin konstin added the preview Experimental behavior label May 14, 2024
/// A package in a workspace.
#[derive(Debug, Clone)]
#[cfg_attr(test, derive(serde::Serialize))]
#[allow(dead_code)] // TODO(konsti): Resolve workspace package declarations.
Copy link
Member Author

Choose a reason for hiding this comment

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

We want this information when resolving workspace dependencies, but we don't use it yet

fn albatross_in_example() {
let (project, root_escaped) = workspace_test("albatross-root-workspace");
let filters = vec![(root_escaped.as_str(), "[ROOT]")];
insta::with_settings!({filters => filters}, {
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried the json snapshots here for the first time and find them better than the regular debug snapshots

use std::collections::HashMap;
use std::collections::BTreeMap;
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes the test snapshots deterministic

@konstin konstin force-pushed the konsti/discover-workspaces-without-using-them branch 2 times, most recently from 18b6467 to ae5d2a3 Compare May 15, 2024 21:54
Copy link
Member Author

Choose a reason for hiding this comment

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

@konstin konstin force-pushed the konsti/discover-workspaces-without-using-them branch from 4e61365 to 8220fff Compare May 16, 2024 13:31
Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Could you add some documentation explaining what's going on? Or at least link to the relevant "specification" we're implementing here? I'm having a hard time reviewing because there's a lot of behavior in here.

@konstin
Copy link
Member Author

konstin commented May 17, 2024

I've edited the spec from #3404 into a docstring

@konstin konstin force-pushed the konsti/discover-workspaces-without-using-them branch 2 times, most recently from e1c2b07 to d3dbef4 Compare May 21, 2024 11:35
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

If this is causing you trouble, you can merge and I'll do a post-push review.

@konstin konstin force-pushed the konsti/discover-workspaces-without-using-them branch 4 times, most recently from 5538a20 to 6c5cf7f Compare May 21, 2024 17:04
@konstin
Copy link
Member Author

konstin commented May 21, 2024

Merging this now to avoid merge conflicts

@konstin konstin enabled auto-merge (squash) May 21, 2024 17:05
@konstin konstin force-pushed the konsti/discover-workspaces-without-using-them branch from 6c5cf7f to a0b9ea2 Compare May 21, 2024 17:10
@konstin konstin merged commit 2ffd453 into main May 21, 2024
43 of 44 checks passed
@konstin konstin deleted the konsti/discover-workspaces-without-using-them branch May 21, 2024 17:17
konstin added a commit that referenced this pull request May 21, 2024
I don't really understand why this only happens on windows clippy and
not on linux too, but as usual, boxing the error variant fixes it.

Fixup for #3585
charliermarsh added a commit that referenced this pull request May 21, 2024
## Summary

This is just a re-apply of #3696, which @konstin accidentally reverted
in #3585.

Co-authored-by: konsti <[email protected]>
@charliermarsh
Copy link
Member

@konstin - Gave this a read, looks like a good start.

.map(|workspace| (project_path.clone(), workspace.clone(), project.clone()));

if workspace.is_none() {
workspace = find_workspace(&project_path)?;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an or_else?

Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we're in a member of a workspace?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, added some docs in the other branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Experimental behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants