Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feat: Add VSCodium Reporter #173

Closed
wants to merge 2 commits into from
Closed

Feat: Add VSCodium Reporter #173

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b100e5b
Add VSCodium Reporter
stevendkwtz Apr 5, 2024
97b2ce9
rename vscodiumReporter to codiumReporter
stevendkwtz Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions lib/Reporting/Reporters/vscodiumReporter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
"use strict";

var autils = require("../../AUtils");
var GenericDiffReporterBase = require("../GenericDiffReporterBase");
var ostools = require("../../osTools");

class Reporter extends GenericDiffReporterBase {
constructor() {
super("codium");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change this to codiumReporter.js, would prefer a human to advise though, not familiar with this codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: done


if (ostools.platform.isWindows) {
this.exePath = autils.searchForExecutable("codium.cmd");
} else {
this.exePath = autils.searchForExecutable("codium");
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This smells like a bad suggestion? I only see multireporter throwing

}

report(approved, received, options) {
autils.createEmptyFileIfNotExists(approved);

options.cmdArgs = ["-n", "--diff", received, approved];
options.cmdOptionOverrides = {
detached: true,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

return super.report(approved, received, options);
}
}

module.exports = Reporter;
1 change: 1 addition & 0 deletions readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ Any of the following reporter may suite your needs. Some support images, some on
| `vimdiff` | | ✓ | ✓ | |
| `visualstudio` | [Visual Studio Diff tool](http://msdn.microsoft.com/en-us/library/bb385990.aspx) | ✓ | |
| `vscode` | [Visual Studio Code Diff tool](https://code.visualstudio.com/) | ✓ | ✓ |
| `vscodium` | [VSCodium Diff tool](https://vscodium.com/) | ✓ | ✓ |


### Custom Reporter
Expand Down