Throw an exception on JXA failure on MacOS #57
Conversation
Under this change, if `/usr/bin/osascript -l JavaScript ...` exits with a nonzero code then a RuntimeError is raised (AlJohri#56). This avoids convert() failing silently when used in a script. Test plan: The new unit test checks that a RuntimeError is raised, if it is running on a MacOS system where Microsoft Word is not available. This change needs to be manually tested on a MacOS system where Microsoft Word is available. It should not have affected behaviour on Windows.
Issue AlJohri#56. Instead of raising a RuntimeError with the message from the JXA, which is a little cryptic, instead check for Microsoft Word before even trying to run the JXA, and raise an EnvironmentError if not. Test plan: Updated unit test reflects this. Needs manual testing on a system that has Microsoft Word.
|
@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 my suggestion: put this functionality behind a flag / argument to the when using what do you think? |
|
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 |
|
alternatively, instead of eager checking, is there a way we can catch the error either in JXA or Python when the command fails? |
|
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. |
|
TL;DR as much as possible, I would rather not eager check and instead just propagate the JXA error 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 |
Great! I'll try to write some for Windows when I get my hands on a Windows machine that has Microsoft Word.
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).
Makes sense. In that case I'll rollback that last commit. |
PR AlJohri#57, Issue AlJohri#56 This reverts commit acf494e. Eager checking could be slow especially if convert() is being run on a lot of different functions. Let's just rely on the JXA exiting with a nonzero return code and giving the message "Application can't be found" if and only if Microsoft Word cannot be launched, and raise an EnvironmentError (or OSError) in that case. If JXA exits with a nonzero return code with any other message, raise RuntimeError. See discussion at AlJohri#57 (comment)
...done. |
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.