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

[docs] update to [email protected] #3501

Merged
merged 10 commits into from
Sep 16, 2023
Merged

[docs] update to [email protected] #3501

merged 10 commits into from
Sep 16, 2023

Conversation

odow
Copy link
Member

@odow odow commented Sep 11, 2023

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (8bcea81) 98.13% compared to head (51d04be) 98.14%.
Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3501   +/-   ##
=======================================
  Coverage   98.13%   98.14%           
=======================================
  Files          37       37           
  Lines        5536     5545    +9     
=======================================
+ Hits         5433     5442    +9     
  Misses        103      103           

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odow
Copy link
Member Author

odow commented Sep 12, 2023

@mortenpi Looks like it didn't make a difference. Should I try changing the example size threshold?

This PR

image

Last master

image

@mortenpi
Copy link
Contributor

Should I try changing the example size threshold?

Nah, that's set to 1 KiB. But yeah, I think the issue is that the figures are using the show for text/html MIME, which we can't really write to a file..

docs/make.jl Outdated Show resolved Hide resolved
docs/make.jl Outdated Show resolved Hide resolved
Co-authored-by: Morten Piibeleht <[email protected]>
@odow
Copy link
Member Author

odow commented Sep 12, 2023

Now there are a bunch of
image

@mortenpi
Copy link
Contributor

That's progress! I think? The only pages that now seem to warn about being over the size threshold are the API pages and changelog!

but no supported image representation is present as an alternative.

These ones should be debugged -- why isn't there an alternative MIME for these ones?

@example block is above the threshold, falling back to 'image/svg+xml' representation.

I am not sure about the utility of these warning. My point is that you should be aware that we're picking a suboptimal representation due to size (otherwise users will complain "why is Documenter not making pretty plots"). But also, real-world docs like JuMP will have a lot of these. I think in an ideal world, I would like the user to somehow disable the text/html MIME if they can't use it anyway. But I am not convinced that this is the right way to go.

One option to improve this is to collect all of these and print just a single warning saying that N at-example blocks got replaced with mime X on pages y, z....

docs/make.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Sep 12, 2023

These ones should be debugged -- why isn't there an alternative MIME for these ones?

I think they're the DataFrames? There are no plots in geographic clustering

https://jump.dev/JuMP.jl/previews/PR3501/tutorials/linear/geographic_clustering/

@odow
Copy link
Member Author

odow commented Sep 12, 2023

One option to improve this is to collect all of these and print just a single warning saying that

Yeah that sounds good.

Although I'd also be happy with it just happening. In what situation is text/html a better representation than image/svg+xml?

I wonder if documenter could introduce something that we could pirate. Like default_mime_type(::Any) = MIME("text/html"), and then we could add something like Documenter.default_mime_type(::Plots.Plot) = MIME("image/svg+xml"). Pirating doesn't matter because this is only getting run once. I guess it could also be some sort of argument to Documenter.HTML().

@mortenpi
Copy link
Contributor

I think they're the DataFrames?

Yeah, that would make sense. It looks like some of them are pretty big as well though, going over the 8K limit.

In what situation is text/html a better representation than image/svg+xml?

I did not check explicitly, but e.g. with PlotlyJS? Since you could get SVG + interactivity with text/html.

I wonder if documenter could introduce something that we could pirate. Like default_mime_type(::Any) = MIME("text/html"), and then we could add something like Documenter.default_mime_type(::Plots.Plot) = MIME("image/svg+xml"). Pirating doesn't matter because this is only getting run once. I guess it could also be some sort of argument to Documenter.HTML().

Yeah, something like that could be an option actually. I'll try to remember to open a follow-up issue with this.

docs/make.jl Outdated Show resolved Hide resolved
@mortenpi
Copy link
Contributor

This looks a bit better!

image

@odow
Copy link
Member Author

odow commented Sep 15, 2023

Yip. Just trying the excludes now.

@odow odow changed the title Do Not Merge: test Documenter #2247 [docs] update Documenter version Sep 15, 2023
@odow
Copy link
Member Author

odow commented Sep 15, 2023

Thoughts @mortenpi? I think this is good to merge

@mortenpi
Copy link
Contributor

If you wait a day, you might be able to get rid of the Pkg.pkg"add Documenter#2fabe9c3bd40b6288c3d86d96c6031728b2d92ad" line too 😛 But yeah, I think this looks good to me!

@odow
Copy link
Member Author

odow commented Sep 15, 2023

If you wait a day

Cool cool. That would be good. So close!

@odow odow changed the title [docs] update Documenter version [docs] update to [email protected] Sep 15, 2023
docs/Project.toml Outdated Show resolved Hide resolved
@odow odow merged commit 68f1cd0 into master Sep 16, 2023
@odow odow deleted the odow-patch-1 branch September 16, 2023 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants