-
Notifications
You must be signed in to change notification settings - Fork 480
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
Patch in inventory version in deploydocs
#2443
Conversation
For what it's worth, for testing, I would factor the bit in |
I don't think I can continue with this approach in good conscience. With the Without the I also really don't like that the inventory isn't complete after I don't think it's so terrible if the inventory has its own logic on where to get its I should have started with just reading the Anyway, any thoughts on this from @fredrikekre or @odow? I'm still open to fresh ideas. This is certainly not to just push through my preferred solution. Maybe I'm overlooking something fundamental, but having had a few nights to sleep on this, I've only gotten more and more convinced that #2442 is the right thing to do. It also happens to be the simplest solution, with no change to Documenter's existing API. |
|
And just to be clear: if someone can tell me how we can get a |
I think this was said somewhere, but to double check: does the version have to be a version number (format-wise)? It can't be a random string (e.g. And if not, could we just leave it empty for dev docs? You'd normally link against Strictly speaking, Yes, this would mean that getting a correct version in the inventory for dev versions is something the package authors would have to configure. But, for dev versions, I don't feel like we have strong enough standard practice to safely assume that the Footnotes
|
It's mostly just metadata for human consumption. I don't think anything really breaks depending on the
So with
Technically, sure, but definitely not what I had in mind.
I'm not sure that's true. For intra-org
Every project will have its own policy on that – or lack of policy, in which case the
I don't think that's necessary. We'd already have
Everyone deploys dev-builds and would have to set that up. Or, more likely, not set it up, so that we'd end up with a lot of inventories in the wild that lack full metadata. Which is why I was hoping to avoid everyone having to tweak their We should definitely make sure that Julia-nightly automatically gets the correct version number into their inventory. Julia has some kind of custom setup: they don't have "dev" in the versions-selection menu, but, e.g., The most correct behavior for the inventory |
8e92732
to
5107194
Compare
I did the refactoring and completed the tests. This now behaves as @mortenpi requested:
Since this does not fulfill the minimum requirements of setting a version for |
@mortenpi I understand where you're coming from and wanting to avoid "multiple sources of truth", and I totally respect you having the final word on these architectural decisions. There are technical reasons I don't like the approach in this PR very much: inventory isn't complete after To boil down why I have a different perspective on the "multiple sources of truth" issue (reiterating some of my previous comments):
I understand it's a judgement call, and no hard feelings, but I really hope I can bring you around to this perspective (and maybe @fredrikekre and @odow can still chime in to sway opinions). |
So, I disagree with this pretty strongly. Semantically, they are versions. They just happen to be implemented via subdirectories and symlinks. The assumption that they are versions is baked in pretty strongly.
So, I don't actually disagree with this. But we are stuck with design decisions that predate The second related design decision that is causing us pain is the distinction between So what I am trying to get to here is some solution that doesn't break the existing assumptions, but gives us a working documentation inventory that we could start using in the ecosystem. And then, at some point, we can fix it up when we overhaul the whole
My gut reaction is that if |
One more note: I think I would nearly be okay with However, I don't like the brittleness when it comes to edge cases, where Documenter is not used in the very common, but not universal
Hence the strong preference for an explicit way of setting that version. And writing Footnotes
|
50de121
to
dbb9014
Compare
This sets the `version` for the `objects.inv` inventory file as follows: * If an `inventory_version` is given as an option to `HTML`, use that. Otherwise, the inventory `version` is left blank in the `makedocs` step * In `deploydocs`, if the inventory version is blank, patch in a version derived from deployment folder name. This will generally set the correct version for tagged releases, but result in a blank version for `dev`-deployments.
dbb9014
to
2173b4d
Compare
There's a part of this I agree with, and maybe that's the common ground we've been trying to get to: If the tag/branch/deployment folder is a valid version, then the "project version" (whether it's extracted from a
I agree on not going down that rabbit hole right now, but I actually don't even think it would be a good idea: The versions in the version-selection menu should be derived from git tags, not from the project version in the
I agree with that, but I don't think the situation is that dire, if we just enforce that the version in
As a general rule, I'm allergic to automatic values silently overwriting values that have been explicitly set by a human. But I am okay with throwing an error if
I agree with that 100%, but let's just throw an error and let the user fix their mistake.
Although my general rule is "a human should always have the last word", I don't think we need to allow a situation where the versions-menu shows "v1.0", but the user manually sets the inventory version to "v0.1". That's clearly a mistake.
I've added another #2448 that is just #2443 with the additional fallback.
I don't think that part is very tricky. The way I've implemented this is pretty straightforward:
So, basically, we identify the main This should work for at least 99% of projects. It should work for monorepos, too (although I haven't found any monorepo setups that use Documenter and deploy more than just the documentation of their "main" package). The most common situation where it might fail is for independent documentation repos (where the documentation is in a separate repo from the package). These repos currently might have a I could also imagine that some repos might have a So in the worst case, people will get yelled at that they need to fix their setup and have a So now, we have three competing PRs on top of #2424 to deal with the version setting
@mortenpi Let me know if you have further questions to help you decide between these options, or if there is anything that requires tweaking or refactoring in any one of the three PRs. But besides that, tell me which one of these you decide on, and I'll merge that into #2424 and wrap everything up with a CHANGELOG entry and such. |
I turned #2442 (comment) into a fourth candidate PR:
Having slept on this variation another night, it seems like a pretty good compromise, as it takes @mortenpi comment to heart that
This is under the assumption that
is ultimately something you can bring yourself around to. Without any fallback, we'll just have to leave the |
Closing in favor of #2449 (comment) |
Alternative to #2442,
#2433#2448, and #2449.Still work in progress (a little difficult to test).I also think this is an extremely cumbersome approach, and hope we don't go with this one. My preferred alternative at this point would probably be the very simple #2442 or #2449.