ASML-Labs / PPTX.jl

Generate PowerPoint PPTX files from Julia
https://asml-labs.github.io/PPTX.jl/
MIT License
90 stars 7 forks source link

Use `DefaultApplication` #24

Closed this-josh closed 1 year ago

this-josh commented 1 year ago

A small PR which I believe closes #22 ,I've changed open_ppt to use DefaultApplication.

matthijscox-asml commented 1 year ago

Since this line of code is untested automatically... on which operating system did you test this manually?

Edit: I just tested on Windows 10, there it works fine.

codecov[bot] commented 1 year ago

Codecov Report

Merging #24 (845034a) into main (0e15364) will increase coverage by 0.81%. The diff coverage is 0.00%.

:exclamation: Current head 845034a differs from pull request most recent head 7011a40. Consider uploading reports for the commit 7011a40 to get more accurate results

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
+ Coverage   93.56%   94.37%   +0.81%     
==========================================
  Files           9        9              
  Lines         466      462       -4     
==========================================
  Hits          436      436              
+ Misses         30       26       -4     
Impacted Files Coverage Δ
src/write.jl 85.98% <0.00%> (+3.09%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

this-josh commented 1 year ago

I tested it on MacOS 13.1, testing it in CI is rather tricky. https://github.com/tpapp/DefaultApplication.jl#testing

matthijscox-asml commented 1 year ago

Yes, let's assume the package just works and keep the CI simple here.

I'm wondering whether to add back the try/catch and throw a warning instead of an error when the file cannot open, one user already got confused since it seemed that the writing failed. On the other hand, that's more code to (not) test. So I'm okay to trust DefaultApplication.jl for now.

Feel free to version bump to v0.5.3, then I'll register right after merging this PR.

this-josh commented 1 year ago

Sure.

It could be best to change the if statement to

    if open_ppt && isfile(filepath)
            DefaultApplication.open(filepath)
    end

and then let the error be handled by DefaultApplication.jl.

matthijscox-asml commented 1 year ago

I'm assuming that if the file writing fails, it errors and never reaches the open_ppt if-statement. Do you foresee a possibility where the file is not written, but we still reach the open_ppt statement?

this-josh commented 1 year ago

I cannot envisage this scenario, but I presumed from

one user already got confused since it seemed that the writing failed

that it has occurred in some edge case, unless the file was written but failed to open because it was corrupted?

It's worth noting that DefaultApplication doesn't raise an error if the file doesn't exist.

matthijscox-asml commented 1 year ago

that it has occurred in some edge case, unless the file was written but failed to open because it was corrupted?

No, what happened was that the file was successfully written, but an opening error was thrown and the user thought the file was not written.

I'll merge and register. Let's assume DefaultApplication works decently enough.

this-josh commented 1 year ago

I see.

I think that is a good plan.