Feat: Add VSCodium Reporter #173
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @stevendkwtz - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
|
||
| class Reporter extends GenericDiffReporterBase { | ||
| constructor() { | ||
| super("codium"); |
There was a problem hiding this comment.
issue (typo): The constructor argument should match the reporter name 'vscodium', not 'codium'.
This ensures consistency and avoids confusion when referencing the reporter by name.
There was a problem hiding this comment.
I can change this to codiumReporter.js, would prefer a human to advise though, not familiar with this codebase.
| if (ostools.platform.isWindows) { | ||
| this.exePath = autils.searchForExecutable("codium.cmd"); | ||
| } else { | ||
| this.exePath = autils.searchForExecutable("codium"); | ||
| } |
There was a problem hiding this comment.
suggestion (code_refinement): Consider verifying the existence of the executable path after assignment.
This adds an extra layer of reliability by ensuring the path indeed points to an existing executable before proceeding.
| if (ostools.platform.isWindows) { | |
| this.exePath = autils.searchForExecutable("codium.cmd"); | |
| } else { | |
| this.exePath = autils.searchForExecutable("codium"); | |
| } | |
| if (ostools.platform.isWindows) { | |
| this.exePath = autils.searchForExecutable("codium.cmd"); | |
| if (!fs.existsSync(this.exePath)) throw new Error("Executable not found: codium.cmd"); | |
| } else { | |
| this.exePath = autils.searchForExecutable("codium"); | |
| if (!fs.existsSync(this.exePath)) throw new Error("Executable not found: codium"); | |
| } |
There was a problem hiding this comment.
This smells like a bad suggestion? I only see multireporter throwing
|
|
||
| options.cmdArgs = ["-n", "--diff", received, approved]; | ||
| options.cmdOptionOverrides = { | ||
| detached: true, |
There was a problem hiding this comment.
question (code_clarification): Clarify the need for the 'detached' option in 'cmdOptionOverrides'.
Understanding the specific reason for running the process detached could be beneficial for future maintenance.
See #173 for orginal contribution Co-Authored-By: Clare Macrae <[email protected]> Co-Authored-By: Steven <[email protected]>
|
Thanks very much - we will release this soon! |
Add VSCodium Reporter
VSCodium is the open source binary distribution of Microsoft’s editor VS Code.
It is VS Code without the Microsoft telemetry. If VSCode reporter is supported, this should be extremely straightforward to implement, just changing the CLI name from code to codium which I've done.
If you have some suggestions for testing functionaltiy or anything I missed, please let me know. I just put this together quickly because I'm working in a different project which uses this package, and the approvals tool doesn't work with VSCodium, which I use.
Small pull requests are great and easy for me to understand and accept
Please try prefix every commits in the pull request with Arlo's git notation
But it's not small!
Then you should setup a remote pairing session with Llewellyn ( [email protected] )
Usually the sessions are between 45-90 minutes.
assuming you still feel it is small, please include
Description
A description of what the PR achieves.
The solution
Outline the implementation.
Any tests that are affected.
Notation
I prefer lots of very small commits prefixed with Arlo's git notation