[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": {}
}
|
Per previous offline discussion, I'm fine with this package being used as a testbed for what kind of configuration will meaningfully improve agent-based or agent-assisted development, so I'll defer to your judgement here. I would like a comment in the file if the format allows it though, explaining what it is and why it's there. (At a high level I really dislike the explosion of "this file needs to be checked in to support just this one specific tool that people may or may not use", and that competing tools have completely different but equally valid configs that we aren't checking in—or would we if other contributors asked?—leading to a repo that reflects the tool choices of individual developers. But I also recognize that we may not have any better alternatives, at least for now.) |
|
@stuartmorgan-g thank you for sharing your thoughts. I agree that having to check in tooling specific files feels like an antipattern. I am also appreciative of you being willing to bend that guidance for experimentation. For now I think the correct bar is for us to see if we can prove to ourselves if this can help the safety and quality of agent authored code and if it can and we can't find a better alternative then we check it in. But the bar is we need to prove at least to @camsim99 and myself that this is helpful. |
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.