[camera_android_camerax] Add a workspace file that contains where jetski can work#11946
[camera_android_camerax] Add a workspace file that contains where jetski can work#11946reidbaker wants to merge 1 commit into
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request introduces a VS Code workspace configuration file for the camera_android_camerax package. Feedback suggests restricting the workspace folders to the package directory itself to preserve encapsulation and modularity, rather than including external paths.
| { | ||
| "folders": [ | ||
| { | ||
| "path": "." | ||
| }, | ||
| { | ||
| "path": "../../../script" | ||
| }, | ||
| { | ||
| "path": "../../../third_party" | ||
| } | ||
| ], | ||
| "settings": {} | ||
| } No newline at end of file |
There was a problem hiding this comment.
Including paths outside of the package directory (such as ../../../script and ../../../third_party) in a package-specific workspace file breaks package encapsulation and modularity. If these shared directories are needed for development, they should be configured in a root-level workspace file rather than within an individual package's workspace. For a package-specific workspace, it is best to limit the scope to the package itself to ensure it remains self-contained and robust to any future directory restructuring. Additionally, this suggestion adds a trailing newline to the file.
{
"folders": [
{
"path": "."
}
],
"settings": {}
}
The point of this pr is to debate if a file like this can be checked in.
Why check in a workspace file. It allows for consistency across teammates. In this case agents and skills that insist that changes not happen outside of a workspace can better help guide an agent but only if we have a consistent definition of workspace. My gut says that we would only need one of these for camerax. One that is for contributors then for an automated agent checking out just the camera_android_camerax will probably be ok. The best argument for a second one would be if we had an agent that was constrained by not being allowed to make developer facing changes and we limited it to code that was less likely to be visible.