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

Allow arbitrary variables to be passed to be stored in User and Internal #864

Merged
merged 3 commits into from
Nov 10, 2018

Conversation

adamslc
Copy link
Contributor

@adamslc adamslc commented Oct 23, 2018

This PR is the start of an attempt to make Documenter support passing arbitrary arguments from makedocs through to a Writer. So far, I have just removed a few of the HTML specific variables from User, but, if people this that this is a reasonable approach, I will continue to strip away the special case variables.

One downside to this approach as it is currently implemented is that it scatters the default values for variables throughout the code. But this could be resolved with a few get!s at the start of the render function in each writer.

Another potential downside is the performance hit resulting from the untyped dictionary. I haven't done any benchmarking to see if this is significant.

@adamslc adamslc changed the title [WIP/RFC] Allow arbitrary variables to be passed to Writers through makedocs [WIP/RFC] Allow arbitrary variables to be passed to be stored in User and Internal Oct 25, 2018
@adamslc
Copy link
Contributor Author

adamslc commented Oct 25, 2018

I've updated the PR to make the same change to the internal struct. This way, extensions to Documenter can use the Document to store their state as well. As an example of when this is useful, take a look at Citations.jl.

@adamslc adamslc changed the title [WIP/RFC] Allow arbitrary variables to be passed to be stored in User and Internal [RFC] Allow arbitrary variables to be passed to be stored in User and Internal Oct 29, 2018
@adamslc
Copy link
Contributor Author

adamslc commented Oct 29, 2018

This is in a state where it can be merged.

Now the question is if this is the right approach to allowing external components to interact with the internal Documenter pipeline. I'm not particularly tied to this solution, but I do think that something to this effect is needed.

@mortenpi
Copy link
Member

Apologies for not getting to this earlier and thanks for bringing this up! I do think this will require some bikeshedding. Here are some general comments that may or may not be relevant:

  • We should do something about the html_* keywords. Although actually, this PR doesn't make any user-facing changes, so that is probably out of the scope for this PR.
  • Helping out plugin packages is definitely something we want to do. Citations looks awesome and I am very keen to hear what roadblocks you run into and what you need from Documenter to implement such a package.
  • It would be nice to have the default values defined in one place (ideally somewhere in the HTMLWriter module for the html_* variables).
  • I have some thoughts about where the user API should be going. It is in no way a final design, but I was playing around with some ideas at some point. (Also roadmap and CLI for make.jl #75 are tangentially related.)

So I think that I would ideally pass a struct like HTML(canonical="...") for the "non-global" variables. We could maybe then have another struct Plugin, which could simply be a wrapper around a Dict internally?

makedocs(
    sitename = "...",
    HTML(canonical = "..."), # equiv to current html_canonical="..."
    Plugin(foo = "bar"),
)

Or better yet, maybe a struct for every plugin? Documenter could define an abstract type for this which could be used to filter the args of makedocs. I.e. CitationsPlugin <: DocumenterPlugin and

makedocs(
    sitename = "...",
    HTML(html_canonical = "..."),
    CitationsPlugin(foo = "bar"),
    SomeOtherPlugin(baz = "qux"),
)

I think there is some value in namespacing these things, as you might get name clashes otherwise if you have multiple plugins.

We also need some methods acting on the Document object for fetching configuration variables I think, to be used internally by the plugins. We shouldn't rely too much on the internal representation of the structs (especially since they should probably be refactored at some point).

I need to think about this a bit more though and would love to hear opinions on this.

@adamslc
Copy link
Contributor Author

adamslc commented Oct 30, 2018

The struct-per-plugin approach seems much better to me. I knocked together a prototype of that system. A few comments about this prototype:

  1. The naming isn't particularly good. I especially don't like plugin_settings, but that can be decided on later.
  2. The defaults for HTMLWriter have been consolidated once again (and arguably in a more sensible place). This also removes the need for the, IMHO ugly, html_ prefixes.
  3. It would be nice to remove the format keyword, and just build for each writer plugin provided to makedocs. We might need another type WriterPlugin <: DocumenterPlugin for this.

I also looked at your proposed interface, and I have a few thoughts about that:

  1. I don't really like passing ARGS to run; it seems a little too magical. I think would be better to explicitly handle command line args in make.jl.
  2. Making the whole process more modular would be a huge win in my mind. This scheme would eliminate the need for point (3) above.

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

I like this! I think we can stick with this general structure. Could just use some docs.

src/Documents.jl Show resolved Hide resolved
src/Documents.jl Outdated Show resolved Hide resolved
src/Documents.jl Outdated Show resolved Hide resolved
src/Documents.jl Outdated Show resolved Hide resolved
src/Documents.jl Outdated Show resolved Hide resolved
src/Documents.jl Outdated Show resolved Hide resolved
@mortenpi mortenpi changed the title [RFC] Allow arbitrary variables to be passed to be stored in User and Internal Allow arbitrary variables to be passed to be stored in User and Internal Oct 31, 2018
@adamslc
Copy link
Contributor Author

adamslc commented Nov 1, 2018

I think that I've updated all of the relevant docstrings, but it is entirely possible that I missed somethings. Also, it looks like there are a lot more things
that can find new homes in plugin types, but hopefully those can be tackled in later PRs?

Copy link
Member

@mortenpi mortenpi 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! A few last things to do and it is good to go I think:

  • We should switch out docs/make.jl over to this.
  • Also the tests under test/formats and test/example



# User Interface.
# ---------------

export Deps, makedocs, deploydocs, hide

export Deps, makedocs, deploydocs, hide, HTML
Copy link
Member

Choose a reason for hiding this comment

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

It turns out that HTML is already being exported by Base, so one needs to do explicit Documenter.HTML. But that's a bit long.. so I am wondering if there is a better way.

Maybe export Writers and have it be accessible by Writers.HTML:

makedocs(
  Writers.HTML(prettyurls=false),
  ...
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted for Documenter.HTML because pretty soon Writers will only have a single writer under it. Also, make.jl files shouldn't be edited too often, so the extra typing seems insignificant.

I can change it if you feel strongly that Writers.HTML is the right way forward.

src/Documents.jl Outdated Show resolved Hide resolved
src/Utilities/Utilities.jl Outdated Show resolved Hide resolved
@adamslc
Copy link
Contributor Author

adamslc commented Nov 7, 2018

I just squashed this into two logical commits. I see you added a news label, but I'm not sure where the news should go? Is there anything else that needs to be done here?

@mortenpi
Copy link
Member

mortenpi commented Nov 7, 2018

I would like to take another proper look at this and I should be able to get to it on the weekend. But I think it is most likely good to go, except for the conflict in src/Documenter.jl.

Add depreciations, update docstrings, fix tests, and export
Documenter.HTML
type is still a reserved keyword on 0.7...
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for iterating on this!

@mortenpi mortenpi merged commit ea55aa5 into JuliaDocs:master Nov 10, 2018
mortenpi added a commit that referenced this pull request Dec 1, 2018
mortenpi added a commit that referenced this pull request Dec 2, 2018
"""
function getplugin(doc::Document, plugin_type::Type{T}) where T <: Plugin
if !haskey(doc.plugins, plugin_type)
doc.plugins[plugin_type] = plugin_type()
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to return nothing here instead of assuming that T has a T() constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rational was to make the interface as easy as possible from the user's perspective. With this implementation, a user can just using SomePlugin in make.jl and things will Just Work (tm).

@fredrikekre
Copy link
Member

Do you have an example implementation that make use of this @adamslc ?

@adamslc
Copy link
Contributor Author

adamslc commented Dec 2, 2018

The `HTMLWriter now uses a plugin, and I have a prototype of a Citation package that uses this feature here.

@adamslc adamslc deleted the userparams branch December 2, 2018 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants