Add MSG to EML email conversion support (#367) #370
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add new msgconvert converter using libemail-outlook-message-perl - Support conversion from Outlook MSG files to standard EML format - Add msgconvert to Docker dependencies and version checking - Register msgconvert converter in main converter registry Implements feature request C4illin#367 for email format conversion
Reviewer's GuideThis PR implements Outlook MSG to EML conversion by integrating the Perl-based msgconvert tool: it adds a new converter module with execution logic, registers it in the main registry, ensures the tool is installed in Docker, and surfaces its version in the printVersions helper. Sequence diagram for MSG to EML conversion processsequenceDiagram
participant User
participant ConverterRegistry
participant msgconvert_module as msgconvert
participant msgconvert_tool as msgconvert (CLI)
User->>ConverterRegistry: Request conversion (MSG to EML)
ConverterRegistry->>msgconvert: Call convert()
msgconvert->>msgconvert_tool: Execute msgconvert CLI
msgconvert_tool-->>msgconvert: Conversion result
msgconvert-->>ConverterRegistry: Done
ConverterRegistry-->>User: Conversion complete
Class diagram for updated converter registry in main.tsclassDiagram
class ConverterRegistry {
+pandoc
+msgconvert
+dvisvgm
...
}
class msgconvert {
+properties
+convert()
}
ConverterRegistry --> msgconvert : registers
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @gingeekrishna - I've reviewed your changes - here's some feedback:
- Align the export names in msgconvert.ts with how you import them in main.ts (e.g. export convertMsgconvert or adjust the registry import) to avoid naming mismatches.
- Improve error handling by rejecting with Error objects (instead of plain strings) and resolving with the actual output file path rather than a static "Done" message for better downstream usage.
- Consider extracting the execFile promise-handling boilerplate into a shared helper to reduce duplication across converters and simplify future additions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Align the export names in msgconvert.ts with how you import them in main.ts (e.g. export convertMsgconvert or adjust the registry import) to avoid naming mismatches.
- Improve error handling by rejecting with Error objects (instead of plain strings) and resolving with the actual output file path rather than a static "Done" message for better downstream usage.
- Consider extracting the execFile promise-handling boilerplate into a shared helper to reduce duplication across converters and simplify future additions.
## Individual Comments
### Comment 1
<location> `src/converters/msgconvert.ts:33` </location>
<code_context>
+ return;
+ }
+
+ if (stdout) {
+ console.log(`stdout: ${stdout}`);
+ }
+
</code_context>
<issue_to_address>
Logging stdout may be unnecessary and could clutter output.
If there's no specific reason to log msgconvert's stdout, please remove this console.log to keep production output clean.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (stdout) {
console.log(`stdout: ${stdout}`);
}
=======
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/converters/msgconvert.ts:37` </location>
<code_context>
+ console.log(`stdout: ${stdout}`);
+ }
+
+ if (stderr) {
+ console.error(`stderr: ${stderr}`);
+ }
+
</code_context>
<issue_to_address>
Logging stderr directly may expose internal details.
Consider logging stderr conditionally or sanitizing its content to avoid exposing sensitive information.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (stderr) {
console.error(`stderr: ${stderr}`);
}
=======
if (stderr) {
// Sanitize stderr to avoid exposing sensitive information
const sanitizedStderr = stderr.replace(/(\/[^\s]+)/g, "[REDACTED_PATH]");
// Optionally, log a generic message or the sanitized content
console.error(`stderr: ${sanitizedStderr.length > 200 ? sanitizedStderr.slice(0, 200) + '...' : sanitizedStderr}`);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/converters/msgconvert.ts:41` </location>
<code_context>
+ console.error(`stderr: ${stderr}`);
+ }
+
+ resolve("Done");
+ });
+ } else {
</code_context>
<issue_to_address>
Returning a static string on success may be limiting.
Consider returning targetPath or relevant result data instead to provide more useful context for callers and improve future flexibility.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
resolve("Done");
=======
resolve(targetPath);
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/converters/msgconvert.ts
Outdated
Comment on lines
33
to
35
| if (stdout) { | ||
| console.log(`stdout: ${stdout}`); | ||
| } |
There was a problem hiding this comment.
suggestion: Logging stdout may be unnecessary and could clutter output.
If there's no specific reason to log msgconvert's stdout, please remove this console.log to keep production output clean.
Suggested change
| if (stdout) { | |
| console.log(`stdout: ${stdout}`); | |
| } |
src/converters/msgconvert.ts
Outdated
Comment on lines
37
to
39
| if (stderr) { | ||
| console.error(`stderr: ${stderr}`); | ||
| } |
There was a problem hiding this comment.
🚨 suggestion (security): Logging stderr directly may expose internal details.
Consider logging stderr conditionally or sanitizing its content to avoid exposing sensitive information.
Suggested change
| if (stderr) { | |
| console.error(`stderr: ${stderr}`); | |
| } | |
| if (stderr) { | |
| // Sanitize stderr to avoid exposing sensitive information | |
| const sanitizedStderr = stderr.replace(/(\/[^\s]+)/g, "[REDACTED_PATH]"); | |
| // Optionally, log a generic message or the sanitized content | |
| console.error(`stderr: ${sanitizedStderr.length > 200 ? sanitizedStderr.slice(0, 200) + '...' : sanitizedStderr}`); | |
| } |
- Remove unnecessary stdout logging to reduce output clutter - Sanitize stderr logging to protect sensitive path information - Return targetPath instead of generic 'Done' message for better caller context - Use proper Error objects instead of string rejections - Address Sourcery AI feedback from PR C4illin#370
|
Thanks for this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements feature request #367 for email format conversion
Summary by Sourcery
Add support for converting Outlook MSG files to standard EML format using the msgconvert tool.
New Features:
Enhancements:
Build: