Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make all errors in start warnings, actually throw when calling save_payload #18

Merged
merged 8 commits into from
Mar 5, 2024

Conversation

disberd
Copy link
Member

@disberd disberd commented Mar 4, 2024

This PR tries to fix JuliaPlots/PlotlyJS.jl#482

For consistency, now all errors encountered during start() are just sent as warning and only thrown when calling save_payload.

This should not break precompilation of PlotlyJS but the actual error message will not be thrown in PlotlyJS as a custom savefig implementation is used (which does not call PlotlyKaleido.savefig)

Only sends warning during `start()`
@disberd disberd requested a review from BeastyBlacksmith March 4, 2024 14:52
@disberd
Copy link
Member Author

disberd commented Mar 5, 2024

I ended up removing the variable storing the error message and defaulting to @warn instead of error.

Now a more descriptive error is thrown inside save_payload if the process is not running when the function is called (the function will anyway throw with broken pipe error if the startup failed before this PR)

@disberd disberd requested a review from BeastyBlacksmith March 5, 2024 10:51
Copy link
Member

@BeastyBlacksmith BeastyBlacksmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@disberd
Copy link
Member Author

disberd commented Mar 5, 2024

Thanks @BeastyBlacksmith. I'll then merge a publish a new non-breaking release to fix PlotlyJS precompilation

@disberd disberd merged commit 62a4cd2 into JuliaPlots:main Mar 5, 2024
6 checks passed
@disberd disberd deleted the move_errors_savefig branch March 5, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows error for PlotlyKaleido breaks precompilation
2 participants