refactor(index): use path.dirname over regex to resolve bin path#548
refactor(index): use path.dirname over regex to resolve bin path#548
path.dirname over regex to resolve bin path#548Conversation
There was a problem hiding this comment.
Pull request overview
Refactors how the UnRTF binary installation directory is derived from which/where output by using Node’s path.dirname() instead of a custom regex, aiming for more robust path handling.
Changes:
- Import
dirnamefromnode:path. - Remove the cached
UNRTF_PATH_REGregex used to parsewhich/whereoutput. - Compute
#unrtfPathviadirname()of trimmedwhich/wherestdout, and adjust the win32 fallback condition accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ]) | ||
| .stdout.toString() | ||
| .trim(); | ||
| if (which) { | ||
| this.#unrtfPath = dirname(which); | ||
| } |
There was a problem hiding this comment.
if (which) { this.#unrtfPath = dirname(which); } will also treat any non-empty diagnostic message on stdout as a path (e.g., some where/which failure outputs), resulting in dirname(...) === "." and a misleading later "Unable to determine UnRTF version" error. Consider gating this assignment on spawnSync(...).status === 0 and/or validating that the selected line is an absolute/expected executable path before setting #unrtfPath.
| const which = spawnSync(platform === "win32" ? "where" : "which", [ | ||
| "unrtf", | ||
| ]).stdout.toString(); | ||
| const unrtfPath = UNRTF_PATH_REG.exec(which)?.[1]; | ||
|
|
||
| if (unrtfPath) { | ||
| this.#unrtfPath = unrtfPath; | ||
| ]) | ||
| .stdout.toString() | ||
| .trim(); | ||
| if (which) { | ||
| this.#unrtfPath = dirname(which); | ||
| } | ||
|
|
||
| /* istanbul ignore next: requires specific OS */ | ||
| if (platform === "win32" && !unrtfPath) { | ||
| if (platform === "win32" && !this.#unrtfPath) { |
There was a problem hiding this comment.
The constructor logic now depends on parsing where/which output with trimming + dirname(), but the existing tests only cover empty stdout and a single-path stdout. Add a unit test that mocks win32 where returning multiple lines and/or a non-empty "not found" message, to ensure #unrtfPath is set (or remains empty to trigger the win32 fallback) correctly.
| const which = spawnSync(platform === "win32" ? "where" : "which", [ | ||
| "unrtf", | ||
| ]).stdout.toString(); | ||
| const unrtfPath = UNRTF_PATH_REG.exec(which)?.[1]; | ||
|
|
||
| if (unrtfPath) { | ||
| this.#unrtfPath = unrtfPath; | ||
| ]) | ||
| .stdout.toString() | ||
| .trim(); | ||
| if (which) { | ||
| this.#unrtfPath = dirname(which); | ||
| } |
There was a problem hiding this comment.
where on Windows can return multiple matching paths (one per line). Calling dirname() on the raw multi-line string will produce an invalid path and can prevent the win32 fallback from running. Consider splitting stdout by newlines and selecting the first valid path (or the first line that ends with unrtf/unrtf.exe) before applying dirname().
c762644 to
5b727e2
Compare
Purpose-built path utility should be much more robust than my regex!
Checklist
npm test