Conversation
Parse [tool.uv.workspace] member and exclude globs from pyproject.toml to discover all workspace member manifests. Virtual workspaces (no [project] section) correctly exclude the root pyproject.toml. Adds __pycache__ and .venv to default ignore patterns. TC-4266 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
discoverUvWorkspaceMembers,manifestsis anArrayListbut lateraddFirst(rootPyproject)is called, which will not compile; consider usingmanifests.add(0, rootPyproject)or switchingmanifeststo aLinkedList. - The
Files.walk(workspaceDir)indiscoverUvWorkspaceMembersdoes not use the ignore patterns to prune traversal (they are only applied after discovery viaWorkspaceUtils.filterByIgnorePatterns), which could lead to unnecessary scanning of large directories likenode_modulesor.git; consider incorporating ignore patterns into the walk itself.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `discoverUvWorkspaceMembers`, `manifests` is an `ArrayList` but later `addFirst(rootPyproject)` is called, which will not compile; consider using `manifests.add(0, rootPyproject)` or switching `manifests` to a `LinkedList`.
- The `Files.walk(workspaceDir)` in `discoverUvWorkspaceMembers` does not use the ignore patterns to prune traversal (they are only applied after discovery via `WorkspaceUtils.filterByIgnorePatterns`), which could lead to unnecessary scanning of large directories like `node_modules` or `.git`; consider incorporating ignore patterns into the walk itself.
## Individual Comments
### Comment 1
<location path="src/main/java/io/github/guacsec/trustifyda/impl/ExhortApi.java" line_range="1080-1081" />
<code_context>
+ });
+ }
+
+ if (PyprojectTomlUtils.getProjectName(toml) != null) {
+ manifests.addFirst(rootPyproject);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** ArrayList does not support addFirst; this line will not compile or will behave incorrectly if the type changes later.
To fix this, either change `manifests` to a `LinkedList`/`Deque` implementation, or keep it as an `ArrayList` and use `manifests.add(0, rootPyproject);` instead. If maintaining discovery order is important, consider documenting that intent in the surrounding code or comments.
</issue_to_address>
### Comment 2
<location path="src/main/java/io/github/guacsec/trustifyda/impl/ExhortApi.java" line_range="1085-1086" />
<code_context>
+ }
+
+ return WorkspaceUtils.filterByIgnorePatterns(workspaceDir, manifests, ignorePatterns);
+ } catch (Exception e) {
+ LOG.warning("Failed to discover uv workspace members: " + e.getMessage());
+ return Collections.emptyList();
+ }
</code_context>
<issue_to_address>
**suggestion:** Logging only the exception message may make diagnosing discovery failures difficult.
Using only `e.getMessage()` drops the stack trace and root cause, which makes diagnosing workspace discovery failures much harder. Prefer logging the full exception (e.g., `LOG.log(Level.WARNING, "Failed to discover uv workspace members", e)`) so production issues remain debuggable while still returning an empty list.
Suggested implementation:
```java
return WorkspaceUtils.filterByIgnorePatterns(workspaceDir, manifests, ignorePatterns);
} catch (Exception e) {
LOG.warning("Failed to discover uv workspace members: " + e.getMessage());
return Collections.emptyList();
}
```
```java
return WorkspaceUtils.filterByIgnorePatterns(workspaceDir, manifests, ignorePatterns);
} catch (Exception e) {
LOG.log(Level.WARNING, "Failed to discover uv workspace members", e);
return Collections.emptyList();
}
private static final Set<String> DEFAULT_WORKSPACE_DISCOVERY_IGNORE =
```
1. Ensure that `java.util.logging.Level` is imported at the top of `ExhortApi.java`, e.g.:
`import java.util.logging.Level;`
2. If the logger is not a `java.util.logging.Logger` but another logging framework (e.g., SLF4J), adjust the logging call accordingly (for SLF4J it would be `LOG.warn("Failed to discover uv workspace members", e);` instead of using `Level`).
</issue_to_address>
### Comment 3
<location path="src/test/java/io/github/guacsec/trustifyda/impl/UvWorkspaceDiscoveryTest.java" line_range="33-42" />
<code_context>
+ "libs" + File.separator + "core" + File.separator + "pyproject.toml"));
+ }
+
+ @Test
+ void discoverWorkspaceManifests_uvNoLockFile() throws IOException {
+ Path workspaceDir = UV_FIXTURES.resolve("uv_workspace_no_lock").toAbsolutePath().normalize();
+
+ ExhortApi api = new ExhortApi(Mockito.mock(java.net.http.HttpClient.class));
+ List<Path> manifests = api.discoverWorkspaceManifests(workspaceDir, Set.of());
+
+ assertThat(manifests).isEmpty();
+ }
+
+ @Test
+ void discoverWorkspaceManifests_uvNoWorkspaceConfig() throws IOException {
+ Path workspaceDir = UV_FIXTURES.resolve("uv_workspace_no_config").toAbsolutePath().normalize();
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for uv workspace configs with empty or whitespace-only `members`/`exclude` arrays
`discoverUvWorkspaceMembers` explicitly ignores null/blank entries in `members` and `exclude`, and returns an empty result when `memberPatterns` is empty. Existing tests cover no workspace config and normal members, but not the case where `[tool.uv.workspace]` exists and these arrays contain only empty or whitespace-only strings. Please add a fixture and test for that scenario to verify the filtering logic and avoid treating such configs as valid members in future.
Suggested implementation:
```java
@Test
void discoverWorkspaceManifests_uvWhitespaceOnlyMembersAndExclude() throws IOException {
Path workspaceDir =
UV_FIXTURES.resolve("uv_workspace_whitespace_only").toAbsolutePath().normalize();
ExhortApi api = new ExhortApi(Mockito.mock(java.net.http.HttpClient.class));
List<Path> manifests = api.discoverWorkspaceManifests(workspaceDir, Set.of());
assertThat(manifests).isEmpty();
}
}
```
1. Add a new test fixture directory at `src/test/resources/tst_manifests/workspace/uv/uv_workspace_whitespace_only`.
2. Inside that directory, create a `pyproject.toml` (or equivalent workspace config used by `discoverUvWorkspaceMembers`) containing a `[tool.uv.workspace]` section where both `members` and `exclude` are arrays with only empty and/or whitespace-only strings, for example:
- `members = ["", " ", " "]`
- `exclude = ["", " "]`
3. Ensure that this fixture does not contain any other valid member patterns that would cause `discoverUvWorkspaceMembers` to return non-empty `memberPatterns`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1080
to
+1081
| if (PyprojectTomlUtils.getProjectName(toml) != null) { | ||
| manifests.addFirst(rootPyproject); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): ArrayList does not support addFirst; this line will not compile or will behave incorrectly if the type changes later.
To fix this, either change manifests to a LinkedList/Deque implementation, or keep it as an ArrayList and use manifests.add(0, rootPyproject); instead. If maintaining discovery order is important, consider documenting that intent in the surrounding code or comments.
Comment on lines
+1085
to
+1086
| } catch (Exception e) { | ||
| LOG.warning("Failed to discover uv workspace members: " + e.getMessage()); |
Contributor
There was a problem hiding this comment.
suggestion: Logging only the exception message may make diagnosing discovery failures difficult.
Using only e.getMessage() drops the stack trace and root cause, which makes diagnosing workspace discovery failures much harder. Prefer logging the full exception (e.g., LOG.log(Level.WARNING, "Failed to discover uv workspace members", e)) so production issues remain debuggable while still returning an empty list.
Suggested implementation:
return WorkspaceUtils.filterByIgnorePatterns(workspaceDir, manifests, ignorePatterns);
} catch (Exception e) {
LOG.warning("Failed to discover uv workspace members: " + e.getMessage());
return Collections.emptyList();
} return WorkspaceUtils.filterByIgnorePatterns(workspaceDir, manifests, ignorePatterns);
} catch (Exception e) {
LOG.log(Level.WARNING, "Failed to discover uv workspace members", e);
return Collections.emptyList();
}
private static final Set<String> DEFAULT_WORKSPACE_DISCOVERY_IGNORE =- Ensure that
java.util.logging.Levelis imported at the top ofExhortApi.java, e.g.:
import java.util.logging.Level; - If the logger is not a
java.util.logging.Loggerbut another logging framework (e.g., SLF4J), adjust the logging call accordingly (for SLF4J it would beLOG.warn("Failed to discover uv workspace members", e);instead of usingLevel).
Comment on lines
+33
to
+42
| @Test | ||
| void discoverWorkspaceManifests_uvRootPackageWorkspace() throws IOException { | ||
| Path workspaceDir = UV_FIXTURES.resolve("uv_workspace").toAbsolutePath().normalize(); | ||
|
|
||
| ExhortApi api = new ExhortApi(Mockito.mock(java.net.http.HttpClient.class)); | ||
| List<Path> manifests = api.discoverWorkspaceManifests(workspaceDir, Set.of()); | ||
|
|
||
| assertThat(manifests).hasSize(3); | ||
| assertThat(manifests).allMatch(p -> p.toString().endsWith("pyproject.toml")); | ||
| assertThat(manifests.getFirst()).isEqualTo(workspaceDir.resolve("pyproject.toml")); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test for uv workspace configs with empty or whitespace-only members/exclude arrays
discoverUvWorkspaceMembers explicitly ignores null/blank entries in members and exclude, and returns an empty result when memberPatterns is empty. Existing tests cover no workspace config and normal members, but not the case where [tool.uv.workspace] exists and these arrays contain only empty or whitespace-only strings. Please add a fixture and test for that scenario to verify the filtering logic and avoid treating such configs as valid members in future.
Suggested implementation:
@Test
void discoverWorkspaceManifests_uvWhitespaceOnlyMembersAndExclude() throws IOException {
Path workspaceDir =
UV_FIXTURES.resolve("uv_workspace_whitespace_only").toAbsolutePath().normalize();
ExhortApi api = new ExhortApi(Mockito.mock(java.net.http.HttpClient.class));
List<Path> manifests = api.discoverWorkspaceManifests(workspaceDir, Set.of());
assertThat(manifests).isEmpty();
}
}- Add a new test fixture directory at
src/test/resources/tst_manifests/workspace/uv/uv_workspace_whitespace_only. - Inside that directory, create a
pyproject.toml(or equivalent workspace config used bydiscoverUvWorkspaceMembers) containing a[tool.uv.workspace]section where bothmembersandexcludeare arrays with only empty and/or whitespace-only strings, for example:members = ["", " ", " "]exclude = ["", " "]
- Ensure that this fixture does not contain any other valid member patterns that would cause
discoverUvWorkspaceMembersto return non-emptymemberPatterns.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[tool.uv.workspace]member and exclude globs frompyproject.tomlto discover workspace member manifests[project]section) correctly exclude rootpyproject.tomlfrom results__pycache__and.venvto default ignore patternsPyprojectTomlUtilsandWorkspaceUtilsfor TOML parsing and ignore filteringTest plan
TC-4266
🤖 Generated with Claude Code
Summary by Sourcery
Add support for discovering Python uv workspaces during workspace manifest resolution and extend default ignore patterns.
New Features:
Enhancements:
Tests: