-
-
Couldn't load subscription status.
- Fork 348
Workspace Support #2073
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
base: master
Are you sure you want to change the base?
Workspace Support #2073
Conversation
…aces # Conflicts: # tests/dep/test_sync.py
… failing to match because of new line
Co-authored-by: Ofek Lev <[email protected]>
Co-authored-by: Ofek Lev <[email protected]>
# Conflicts: # tests/cli/self/test_self.py
… failing to match because of new line
|
The diff should be much cleaner now with the additional merge I did from master. |
…ronment level is.
…tion warnings, remove virtualenv upper bounds
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.
Thank you so much for picking this up! I'm going to finish reviewing some time today but have you thought about the interaction between packages that are both workspace members and dependencies? I forgot how I handled that in my branch or intended on handling that if I didn't yet.
src/hatch/cli/__init__.py
Outdated
|
|
||
| # Then check pyproject.toml | ||
| pyproject = current / "pyproject.toml" | ||
| if pyproject.exists() and _has_workspace_config(load_toml_file, str(pyproject), "tool.hatch.workspace"): |
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.
We never actually use the config_path parameter here. I'd suggest that we remove the helper function and just load in this function directly or keep the helper function if you're concerned about random errors and have it return an empty dictionary instead.
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 primary reason for doing this was just to catch errors separately for the file loading to check if there is a workspace configuration. There might be a better way to do this but this seemed clean to avoid catching errors of the same type that might not be related to the file loading.
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.
Let me know if you think this should still be changed
There was not appropriate logic yet for handling conflicts and ensuring order. This is one idea for handling conflicts that could happen and then to ensure workspace members take precedence in virtual environments the sync would look like this Let me know your thoughts on this approach. |
…dencies. Fix formatting issue of workflow file.
This PR finalizes implementation of workspaces. Some of the diff appears to be because I had the same commits locally that have already been merged for the failing tests.
The model that I went with was the same as what uv makes available so users can define
Things to note for feedback, spend a lot of time trying to ensure that every dependency was properly converted from the Requirement class to the child Dependency class and could not find where it kept leaking through as a Requirement instance. The result is that there are a few places where I used list comprehension to convert the entire complex list before returning it.
The workspace functionality test file definitely need a better name than configuration which is the name currently under tests/workspace/
closes: #1639