Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds minVersion: "0.75.0" to the printVersionInfo and replace options in the pdfAttach function configuration. According to the PR description, the pdfattach binary was introduced with Poppler version 0.75.0, so these options (and the entire binary) require at least this version.
Changes:
- Added
minVersion: "0.75.0"toprintVersionInfoandreplaceoptions in the pdfAttach options object
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| minVersion: "0.75.0", | ||
| }, | ||
| replace: { | ||
| arg: "-replace", | ||
| type: "boolean", | ||
| minVersion: "0.75.0", |
There was a problem hiding this comment.
Adding minVersion to these options is correct, but the pdfAttach function (line 1248-1254) doesn't retrieve the version before calling parseOptions(), which means the minVersion validation will never execute. The function should call this.#getVersion(this.#pdfAttachBin) before parsing options, similar to how other functions like pdfFonts, pdfImages, pdfToPpm, etc. handle this (see lines 1288-1289, 1307-1308 for examples). Without this version retrieval, users could attempt to use these options with older Poppler versions that don't support them, and the error won't be caught.
| minVersion: "0.75.0", | |
| }, | |
| replace: { | |
| arg: "-replace", | |
| type: "boolean", | |
| minVersion: "0.75.0", | |
| }, | |
| replace: { | |
| arg: "-replace", | |
| type: "boolean", |
pdfattach binary was introduced with Poppler 0.75.0. See https://gitlab.freedesktop.org/poppler/poppler/-/blob/master/NEWS#L1475
Checklist
npm test