search

JuliaPlots / PlotlyJS.jl

Julia library for plotting with plotly.js
Other
413 stars 77 forks source link

substitute old savefig code with PlotlyKaleido #481

Closed BeastyBlacksmith closed 4 months ago

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 75.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 28.07%. Comparing base (457befe) to head (7481ad7). Report is 1 commits behind head on master.

Files Patch % Lines
src/PlotlyJS.jl 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #481 +/- ## ========================================== - Coverage 35.62% 28.07% -7.55% ========================================== Files 8 8 Lines 306 260 -46 ========================================== - Hits 109 73 -36 + Misses 197 187 -10 ``` | [Flag](https://app.codecov.io/gh/JuliaPlots/PlotlyJS.jl/pull/481/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPlots) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/JuliaPlots/PlotlyJS.jl/pull/481/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPlots) | `28.07% <75.00%> (-7.55%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaPlots#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thchr commented 4 months ago

Would this fix https://github.com/JuliaPlots/PlotlyJS.jl/issues/473? Seems Windows still hangs on precompilation :(

BeastyBlacksmith commented 4 months ago

Well, it doesn't hang on precompilation, but errors at using time

thchr commented 4 months ago

For extensions that pull in PlotlyJS.jl, this seems to be essentially an equivalent problem though?

BeastyBlacksmith commented 4 months ago

In that it doesn't work on windows, yes.

disberd commented 4 months ago

@BeastyBlacksmith, is there a reason why this PR didn't use savefig directly from PlotlyKaleido instead of basically reimplementing the same method?

I had a quick look at the code here and didn't see too much difference.

Edit: I see that it might be due to the html show part... though it's a pity to have the duplication

BeastyBlacksmith commented 4 months ago

though it's a pity to have the duplication

Yeah, they differed slightly, so I didn't want to touch it. But if you have a way to streamline that, feel free to go for it.