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

Add uv-workspace crate with settings discovery and deserialization #3007

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Apr 12, 2024

Summary

This PR adds basic struct definitions along with a "workspace" concept for discovering settings. (The "workspace" terminology is used to match Ruff; I did not invent it.)

A few notes:

  • We discover any pyproject.toml or uv.toml file in any parent directory of the current working directory. (We could adjust this to look at the directories of the input files.)
  • We don't actually do anything with the configuration yet; but those PRs are large and I want this to be reviewed in isolation.

@charliermarsh charliermarsh force-pushed the charlie/workspace branch 4 times, most recently from ba01cd1 to 9f3fe96 Compare April 12, 2024 19:42
@charliermarsh charliermarsh force-pushed the charlie/workspace branch 4 times, most recently from 87b0f7f to b331ed5 Compare April 15, 2024 21:05
@charliermarsh charliermarsh requested review from zanieb and konstin April 15, 2024 21:07
@charliermarsh charliermarsh added the configuration Settings and such label Apr 15, 2024
@charliermarsh charliermarsh marked this pull request as ready for review April 15, 2024 21:07
@@ -105,6 +105,9 @@ async fn run() -> Result<ExitStatus> {
}
};

// Load the workspace settings.
let _ = uv_workspace::Workspace::find(env::current_dir()?)?;
Copy link
Member

Choose a reason for hiding this comment

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

If we don't have read permissions for the current directory should we error or just not attempt to read a workspace there?

Copy link
Member

Choose a reason for hiding this comment

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

I'm for erroring, i'm assuming we have read and write permissions for the entire repo/project and read permissions for dirs above

@@ -105,6 +105,9 @@ async fn run() -> Result<ExitStatus> {
}
};

// Load the workspace settings.
let _ = uv_workspace::Workspace::find(env::current_dir()?)?;
Copy link
Member

Choose a reason for hiding this comment

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

I'm for erroring, i'm assuming we have read and write permissions for the entire repo/project and read permissions for dirs above

/// found.
pub fn find(path: impl AsRef<Path>) -> Result<Option<Self>, WorkspaceError> {
for ancestor in path.as_ref().ancestors() {
if let Some(options) = read_options(ancestor)? {
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 only pick the closest file?

The scenario i'm thinking about is one where the work dir is /home/ferris/foo/bar/ and we have:

  • /home/ferris/uv.toml or /home/ferris/.config/uv.toml: Has some general user-level settings
  • /home/ferris/foo/pyproject.toml: Defines a workspace of python projects and has some project level settings
  • /home/ferris/foo/bar/pyproject.toml: A member of that workspace. Does not contain settings (i think?)

In this case, i want to first load user-level setting, then override them with the project level settings (implicitly), then resolve bar as part of the foo 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 we only pick the closest file. What if we do the same as Ruff, and skip a pyproject.toml that lacks a tool.uv entry?

Copy link
Member

Choose a reason for hiding this comment

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

That'd make sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

root: PathBuf,
}

impl Workspace {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if i'd call that workspace, i'm thinking about having settings, things that are defined e.g. on the user-level, the project level or the cli level which e.g. define indexes, plus having a workspace, an abstraction over a set of packages that are part of single project. That's the main divergence from ruff, in uv (similar to cargo) you're always in a single workspace and/or a single package; i still have to flesh out how to integrate this into uv.

In cargo, you're technically always in a rust workspace, even if you only define a single crate, that's a single crate workspace. Then there's the concept of configuration with hierarchical structure and cli overrides.

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 can call this configuration instead?

Copy link
Member

Choose a reason for hiding this comment

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

I've been intending to call it a "workspace" whether there's one package or many in your project. I don't know if we need to introduce a separate term?

Copy link
Member

Choose a reason for hiding this comment

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

I like configuration

@charliermarsh
Copy link
Member Author

I'm just gonna leave it as "workspace" for now. Let's revisit when we add actual workspace, that's all internal terminology and abstractions anyway.

@charliermarsh charliermarsh merged commit 295b58a into main Apr 16, 2024
38 checks passed
@charliermarsh charliermarsh deleted the charlie/workspace branch April 16, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Settings and such
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants