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

The version argument for makedocs #2385

Open
goerz opened this issue Jan 5, 2024 · 6 comments
Open

The version argument for makedocs #2385

goerz opened this issue Jan 5, 2024 · 6 comments

Comments

@goerz
Copy link
Member

goerz commented Jan 5, 2024

The makedocs function currently takes an undocumented version argument (which the HTML writer can access as ctx.doc.user.version).

I was hoping to start encouraging the use of that argument to pass the version of the package being documented. The inventory files I'm currently prototyping preferably should write this version information into the inventory.

Alas, it turns out that setting the version parameter actually disables the version selection menu entirely (it gets set to "visible", which the javascript then toggles to "invisible").

Is this intentional? The way this happens is pretty round-about, so it may not be intentional. What was the idea behind the version argument when it was originally implemented? Is this being used anywhere in the wild?

Since version is completely undocumented, is there any chance that we can remove

if !isempty(ctx.doc.user.version)
vs_class = "$(vs_class).visible"
opt = option[:value => "#", :selected => "selected", ](ctx.doc.user.version)
vs_select = vs_select(opt)
end

without this being a "breaking change"?

Would it be possible to officially make the version argument to makedocs be for "you can (should) pass the version of the package being documented" and document it as such?

@goerz
Copy link
Member Author

goerz commented Jan 5, 2024

It looks to me like the intent of

if !isempty(ctx.doc.user.version)
vs_class = "$(vs_class).visible"
opt = option[:value => "#", :selected => "selected", ](ctx.doc.user.version)
vs_select = vs_select(opt)
end

is to actively select the version from the version menu. In that case, the line

vs_class = "$(vs_class).visible"

is definitely a bug because of how it interacts with the js later on. I'm also not sure the rest of the code makes much sense. Assuming version is, in fact, the version being deployed, then that version is the one being shown anyway. Deploying documentation but setting version to something else so that that other version would then be selected in the version menu seems like a pretty weird thing to do.

@goerz
Copy link
Member Author

goerz commented Jan 5, 2024

Oh, it turns out version isn't quite undocumented after all! It is mentioned in Documenter.HTMLWriter:

HTMLWriter uses the following additional keyword arguments that can be passed to Documenter.makedocs: authors, pages, sitename, version.
[…]
Experimental keywords
version specifies the version string of the current version which will be the selected option in the version selector. If this is left empty (default) the version selector will be hidden. The special value git-commit sets the value in the output to git:{commit}, where {commit} is the first few characters of the current commit hash.

This description seems obsolete, and not what is actually happening (like, the version selector being hidden by default).

Incidentally, Documenter.LaTeXWriter also takes a version keyword argument with more or less the meaning that I'd like for the inventory. So maybe we should remove the global version and add a version keyword to HTMLWriter (which the inventory can use).

P.S.: I'd even like the HTMLWriter to actually use version at some point in the future, e.g., by putting it in the default footer (or in the navigation bar), but that's a separate issue.

@mortenpi
Copy link
Member

Sorry for the delay here!

is definitely a bug because of how it interacts with the js later on.

I think the bug here is actually in the JS.. the intent of

// only show the version selector if the selector has been populated
if (version_selector_select.children("option").length > 0) {
version_selector.toggleClass("visible");
}

is to make the version selector visible, but it doesn't take into account that the element might already be visible. We should use addClass instead.

But the bigger question is indeed whether we want version at all.. we communicate the package version to the front-end via the siteinfo.js file (e.g for the Julia v1.10.0) (generated by deploydocs). And it pretty much duplicates the information version has, since it gets used as the fallback value in the version selector JS:

// add the current version to the selector based on siteinfo.js, but only if the selector is empty
if (
typeof DOCUMENTER_CURRENT_VERSION !== "undefined" &&
$("#version-selector > option").length == 0
) {
var option = $(
"<option value='#' selected='selected'>" +
DOCUMENTER_CURRENT_VERSION +
"</option>"
);
version_selector_select.append(option);
}

So my current feeling is that we should just fully deprecate version.. it should be relatively non-controversial, since it's half-broken and marked experimental anyway.

For automated tooling, it might be better to post-process .documenter-siteinfo.json (#2181) in deploydocs and add the version number in there? Would that work for the inventory stuff? I am not a huge fan of having to separately configure the version correctly in both deploydocs and makedocs, since determining that from the Git tags etc is non-trivial.

@goerz
Copy link
Member Author

goerz commented Jan 29, 2024

I think the bug here is actually in the JS.. the intent […] is to make the version selector visible, but it doesn't take into account that the element might already be visible. We should use addClass instead.

That sounds right, although I'm still confused about what this would be used for. Is it to force showing a version selection menu when building locally? Wouldn't it conflict with the general logic of what the selector shows? What would happen if I set version to something that's not a subfolder of gh-pages?

[…] it pretty much duplicates the information version has, since it gets used as the fallback value in the version selector JS

So I guess what you're telling me is that setting version just overrides the selector and shows version as the only entry? Is that to make sure that the current version is being shown somewhere if we don't want to keep the documentation for multiple versions around?

So my current feeling is that we should just fully deprecate version.. it should be relatively non-controversial, since it's half-broken and marked experimental anyway.

Yes, I agree. At least in its current form.

@goerz
Copy link
Member Author

goerz commented Jan 29, 2024

For automated tooling, it might be better to post-process .documenter-siteinfo.json (#2181) in deploydocs and add the version number in there? Would that work for the inventory stuff?

No, I don't think so. For one, the inventory should be be completely determined in makedocs and not depend on deploydocs, git tags, CI environment variables, or any such thing.

Actually, I think this goes to an important point: The current version argument to makedocs (as I understand it now), and, e.g., the information in siteinfo.js are really just about the labels shown in the versions-selection menu. In some sense, this is only coincidentally the package version: It is telling that in deploydocs.jl, the generate_siteinfo_file function is called as

HTMLWriter.generate_siteinfo_file(deploy_dir, #=version=#subfolder)

I would consider the "version" that I would need for the inventory to be conceptually different from anything to do with the versions-selection-menu. It is much more like the separate versions parameter that we already have for the LaTeX settings: A human-readable piece of metadata to be shown somewhere in the rendered output.

One detail is that the Sphinx inventory specification specifies that the "version" should not have a "v" prefix. But beyond that, the "version" field can really be used in any way that is most helpful to the user. I'd consider appending a git hash if available, for -dev version, for example.

Oh, and just to expand a little for what the version in the inventory is actually used: Sphinx shows it as a tooltip when hovering over an external link. DocumenterInterLinks currently doesn't use it at all. Maybe I'll figure out how to add a similar tooltip as in Sphinx at some point. In the meantime, it's just metadata for the inventory file, so if someone downloads a objects.inv file, they have some idea what version that inventory relates to.

So maybe the best thing to do would be to add a version parameter to Documenter.HTMLWriter.HTML that mirrors the existing version parameter in Documenter.LaTeXWriter.LaTeX. Right now, in #2424, I can just leave the version string blank. Although, before we make a release, it would be good to have some way for a user to specify what version string should go in the inventory.

I am not a huge fan of having to separately configure the version correctly in both deploydocs and makedocs, since determining that from the Git tags etc is non-trivial.

I think these serve different purposes, one for setting up the versions-selection-menu, and one as metadata to be shown somewhere in the rendered output (which includes the inventory), for human consumption.

In deploydocs, the version information should come from Git tags or folder names, and yes, I think we should deprecate the current version argument to makedocs to play any part in that.

For the version in Documenter.LaTeXWriter.LaTeX and Documenter.HTMLWriter.HTML (for the inventory), these should not be set based on Git tags. They really should be given semi-manually by the package author. I wouldn't mind pushing people towards putting something like

PROJECT_TOML = Pkg.TOML.parsefile(joinpath(@__DIR__, "..", "Project.toml"))
VERSION = PROJECT_TOML["version"]

into their make.jl file (I already always have that because I like to have the VERSION in the footer). If we do want to somehow figure out a default value for these version values, I would do one of these:

  • Search up from the documentation src folder for a Project.toml file the contains a version
  • look at the sitename argument to makedocs, try to extract a package name, and query Pkg for the version of that package in the current manifest.

So at this point, I would propose the following:

  • Deprecate the versions parameter in makedocs and remove it from the documentation. Maybe make a half-hearted attempt to "fix" it by using addClass. I can update Fix version hiding the version selection menu #2389.
  • Merge Write inventory files #2424, temporarily without the ability to have a non-empty version in the objects.inv file
  • Add a parameter version to Documenter.HTMLWriter.HTML. Use it as the way to specify the version in the inventory. Print a warning if version is not set? Clean up the documentation for both Documenter.HTMLWriter and Documenter.LaTeXWriter: These seem pretty outdated. I guess HTML used to be an external plugin at some point? I can start another PR for this. Preferably, it should make it into the same release as Write inventory files #2424.

Maybe in the future:

  • Have an automatic default for version in Documenter.HTMLWriter.HTML and Documenter.LaTeXWriter.LaTeX based on the version in the Project.toml (but not any tag/folder/environment variable that is only available with deploydocs). Remove the default value version=get(ENV, "TRAVIS_TAG", "") in Documenter.LaTeXWriter.LaTeX (already deprecated!). What's our policy for removing deprecated features? I suppose it's breaking, and we have to wait for 2.0 to actually remove them?

@mortenpi
Copy link
Member

mortenpi commented Feb 3, 2024

So at this point, I would propose the following:

  • Deprecate the versions parameter in makedocs and remove it from the documentation. Maybe make a half-hearted attempt to "fix" it by using addClass. I can update Fix version hiding the version selection menu #2389.
  • Merge Write inventory files #2424, temporarily without the ability to have a non-empty version in the objects.inv file
  • Add a parameter version to Documenter.HTMLWriter.HTML. Use it as the way to specify the version in the inventory. Print a warning if version is not set? Clean up the documentation for both Documenter.HTMLWriter and Documenter.LaTeXWriter: These seem pretty outdated. I guess HTML used to be an external plugin at some point? I can start another PR for this. Preferably, it should make it into the same release as Write inventory files #2424.

I think I'm on board with this plan, if we just keep #2424 (comment) in mind.

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 a pull request may close this issue.

2 participants