AlJohri / docx2pdf

MIT License
497 stars 94 forks source link

Throw an exception on JXA failure on MacOS #57

Open jftsang opened 2 years ago

jftsang commented 2 years ago

Covers #56 by raising a RuntimeError (instead of failing silently) when calling the JXA (on MacOS) gives a nonzero return code, rather than failing silently. Also increased test coverage of when the input or output paths do not have the expected file extensions.

AlJohri commented 2 years ago

@jftsang thanks for submitting the PR and adding our first tests!

While it would be great to serve an informative message if "Microsoft Word" is not installed, I think it might be inefficient to always check on each invocation of convert

my suggestion: put this functionality behind a flag / argument to the macos method

when using convert as a function, keep it defaulted to False but when using the CLI, change the flag to default to True

what do you think?

AlJohri commented 2 years ago

in addition, can we split the PR for MS Word not installed and missing docx file for easier review/merging? easier to tackle one new feature/bugfix at a time

AlJohri commented 2 years ago

alternatively, instead of eager checking, is there a way we can catch the error either in JXA or Python when the command fails?

AlJohri commented 2 years ago

one very simple approach could be just getting the error code from the JXA invocation. testing with a small sample script:

#!/usr/bin/osascript -l JavaScript

ObjC.import("stdlib");
SystemEvents = Application("System Events");

const word = Application("Microsoft Word");

if (!word.running()) {
    word.activate();
    delay(1);
    SystemEvents.processes["Microsoft Word"].visible = false;
} else {
    word.launch();
    SystemEvents.processes["Microsoft Word"].visible = false;
}

If I use "Microsoft Word", I get an exit code of 0. When I use a program I don't have installed like "Microsoft Visio" I get an exit code of 1.

AlJohri commented 2 years ago

TL;DR as much as possible, I would rather not eager check and instead just propagate the JXA error

❯ ./test.jxa
./test.jxa: execution error: Error: Error: Application can't be found. (-2700)

❯ echo $?
1

if we want, we can parse the error returned and wrap it with a nicer error that explicitly tells the user "Application can't be found." means Microsoft Word isn't installed

jftsang commented 2 years ago

adding our first tests

Great! I'll try to write some for Windows when I get my hands on a Windows machine that has Microsoft Word.

in addition, can we split the PR for MS Word not installed and missing docx file for easier review/merging? easier to tackle one new feature/bugfix at a time

This PR doesn't do anything to check missing docx, except possibly if the JXA exits with nonzero code on account of being able to launch MS Word but the docx is missing (I don't know if that's the case or not as I don't have MS Word).

if we want, we can parse the error returned and wrap it with a nicer error that explicitly tells the user "Application can't be found." means Microsoft Word isn't installed

Makes sense. In that case I'll rollback that last commit.

jftsang commented 2 years ago

Makes sense. In that case I'll rollback that last commit.

...done.