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

get docs to build again #1283

Merged
merged 7 commits into from
May 18, 2019
Merged

get docs to build again #1283

merged 7 commits into from
May 18, 2019

Conversation

bjarthur
Copy link
Member

@bjarthur bjarthur commented May 5, 2019

fixes #1282

  • I've squash'ed or fixup'ed junk commits with git-rebase
  • I've built the docs and confirmed these changes don't cause new errors

documentation now builds again without errors. i removed the dependency on StatsBase, which changed one unit test in a small way. i also removed some unnecessary using Colors while at it. upgraded to Documenter 0.22. and fixed some formatting in the docs.

@mortenpi could you please explain why in docs/make.jl i have to using Compose, Cairo? even when pages = Any[], i still have to using these or i get errors about them being needed to save image/png.

@mortenpi
Copy link
Contributor

mortenpi commented May 5, 2019

Just a guess here: since v0.22, Documenter renders a bunch of showable MIME types in advance before deciding which one it will actually use. I assume that image/png is showable even when Cairo is not loaded, but won't work until it is loaded?

Project.toml Outdated
test = ["CSV", "Cairo", "DataFrames", "RDatasets", "Test", "Unitful"]
Copy link
Member

Choose a reason for hiding this comment

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

As I understand it, for the line_linestyle test script you could have moved StatsBase into [extras] and [targets], and the docs will build as long as StatsBase is not in [deps]and [compat]. However, I'm ok with removing StatsBase in the line_linestyle testscript, because it simplifies things.

@Mattriks
Copy link
Member

Mattriks commented May 5, 2019

@bjarthur Are you going to look at the issue of Distributions (mentioned by me in #1282) in a separate PR?
I've just noticed that the doc build fails in travis because of Distributions.

@bjarthur
Copy link
Member Author

bjarthur commented May 5, 2019

i'm inclined to remove the entire compat section in Project.toml. thoughts? @non-Jedi ??

my thinking is that we don't know those are the true minimum versions that will work, so there's no sense being restrictive. as we discover that certain versions aren't compatible, we can add them back in. it would be a huge chore to go determine that for each of the dependencies.

@bjarthur
Copy link
Member Author

bjarthur commented May 5, 2019

@mortenpi indeed. those methods are replaced once Cairo is dynamically loaded by Requires. perhaps it's not worth the trouble of getting rid of those usings.

however, i did have another question for you: is there a way to force PNG output, or SVG, instead of SVGJS? this page takes a long time to load because of the embedded javascript.

@Mattriks
Copy link
Member

Mattriks commented May 6, 2019

Re PNG ouput: same issue in GiovineItalia/Compose.jl#228, Compose.set_default_graphic_format(:png) doesn't work.

Re: [compat] I like the idea of removing the [compat] section entirely. Particularly for popular packages (e.g. Colors, Distributions, StatsBase) that users might want to install independently in an environment (along with Gadfly), compatibility issues seem to make installation more difficult. E.g. JuliaLang/Pkg.jl#1131

@mortenpi
Copy link
Contributor

mortenpi commented May 6, 2019

@mortenpi indeed. those methods are replaced once Cairo is dynamically loaded by Requires. perhaps it's not worth the trouble if getting rid of those usings.

The easy option is probably to just have the usings in make.jl. I don't think installing and loading Cairo really hurts the doc build much, even if unnecessary.

But I do wonder now: does it make sense to define show methods that just throw errors? The semantics (according to the default implementation of showable) seem to be that if you have a show for a MIME type defined, then you can show it. I can understand the UX point of view though as well.. but perhaps showable should be overloaded then?

however, i did have another question for you: is there a way to force PNG output, or SVG, instead of SVGJS? this page takes a long time to load because of the embedded javascript.

There is no way to do it in Documenter (although maybe we could have it be an option for the at-blocks), but here is an idea for a workaround: mortenpi@d6bc52e

@mortenpi
Copy link
Contributor

mortenpi commented May 6, 2019

Hmm, a custom showable that checks some global state could be a way to address GiovineItalia/Compose.jl#228 too actually.

@non-Jedi
Copy link
Contributor

non-Jedi commented May 6, 2019

i'm inclined to remove the entire compat section in Project.toml. thoughts? @non-Jedi ??

I mean it's ultimately your call to make. If you don't put that section in, you're basically trusting the packages in question to maintain compatibility forever. Any release you do without a compat section will be broken in the future if any package Gadfly depends on ever makes a breaking change. If you're okay with that and just will update Gadfly whenever breaking changes occur, that's fine, but you're basically opting into a situation where versions of Gadfly that work currently won't necessarily work in the future because of breaking changes on the part of dependencies.

My personal opinion is that a lot of the dependencies of Gadfly that are currently pre-1.0 should just release a 1.0 version so that we can actually abide by the guarantees of semver, which would make this problem mostly go away, but that's not something you or I have any control over.

@codecov-io
Copy link

codecov-io commented May 8, 2019

Codecov Report

Merging #1283 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1283      +/-   ##
==========================================
- Coverage   85.83%   85.81%   -0.03%     
==========================================
  Files          36       36              
  Lines        4158     4158              
==========================================
- Hits         3569     3568       -1     
- Misses        589      590       +1
Impacted Files Coverage Δ
src/theme.jl 71.18% <0%> (-1.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13f5ac6...43b8b3c. Read the comment docs.

Project.toml Outdated
DataFrames = "0.17"
RDatasets = "0.6"
Unitful = "0.15"
# known to work for these, but as it's not an exhaustive list, don't forcefully restrict
Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, if you're going to get rid of the compat section but still want to document versions of packages that work, I'd delete all this and just go ahead and commit a Manifest.toml. That's what that file is for after all.

@bjarthur bjarthur force-pushed the bja/docs branch 2 times, most recently from ae8aa5b to d95eb22 Compare May 9, 2019 00:49
@bjarthur
Copy link
Member Author

bjarthur commented May 9, 2019

how about putting the compat section back in, but removing the upper bounds on the versions for all dependencies whose major version is 0. it's easier i think to fix errors when the plotting code runs, than it is to debug installation problems.

CSV 0.5 doesn't work for Gadfly because it doesn't work for RDatasets. see JuliaStats/RDatasets.jl#69

some dependency of KernelDensity must be incompatible with Distributions 0.19 because adding the former downgrades the latter to 0.18.

oh but wait. now installing on julia 7 fails. and if i take out the [compat] section entirely, it still fails. can anyone get Gadfly master to install on julia 7?

@bjarthur
Copy link
Member Author

ok, master mysteriously works on julia 7 now. not sure why it didn't before. but... this PR does not.
mysteriously though, it works if i change the Gadfly version from 1.1.0 back to 1.0.1. everything is fine on julia 1. at some point we'll probably have to drop support for julia 7, but it'd be nice not to have to do it now.

@bjarthur
Copy link
Member Author

@mortenpi if i understand what you're saying about showable, you'd have me delete the show methods for the Cairo backends in Compose. i've tried that, and can successfully build the Gadlfy docs without using Cairo. importantly, since PDF is still defined to throw an error if Cairo is not loaded, the user will get an informative message.

however, if i understand Documenter correctly, it shows all showable MIME types, despite just choosing one in the end to actually include in the built docs. is the thinking that this is useful because it adds to the test suite? if so, i'm inclined to not try to circumvent that.

@mortenpi
Copy link
Contributor

you'd have me delete the show methods for the Cairo backends in Compose.

Yep. Although in that case if a user does do e.g. show(mime"image/png", ctx), they will just get a missing method error, instead of an helpful message suggesting to install Cairo. So, alternatively, if one wants to keep that, a custom showable would also work I think.

however, if i understand Documenter correctly, it shows all showable MIME types, despite just choosing one in the end to actually include in the built docs. is the thinking that this is useful because it adds to the test suite? if so, i'm inclined to not try to circumvent that.

Testing was not the motivation, although now that you mention it, it is indeed a side-effect. The main reason is that different backends prefer different MIMEs (e.g. HTML likes text/html, but LaTeX can't render that and prefers text/latex). So, as we evaluate the at-example blocks, we generate all available outputs and let the writer pick the one it likes.

@bjarthur bjarthur merged commit 8813880 into GiovineItalia:master May 18, 2019
@bjarthur bjarthur deleted the bja/docs branch May 18, 2019 17:48
@bjarthur bjarthur restored the bja/docs branch May 18, 2019 17:54
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.

Gadfly's Project.toml
5 participants