pydata / pydata-sphinx-theme

A clean, three-column Sphinx theme with Bootstrap for the PyData community
https://pydata-sphinx-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
618 stars 321 forks source link

many users struggle with version switcher/banner handling #1629

Open drammock opened 10 months ago

drammock commented 10 months ago

we use the release value and not the version value from conf.py to decide whether to show a version warning banner. We are assuming folks follow what is stated in the Sphinx docs for release:

If you don’t need the separation provided between version and release, just set them both to the same value.

the fact that folks keep stumbling around the version switcher and the warning banner suggests that either (1) what we're doing is too fragile/magic, or (2) we're not documenting it clearly enough.

Originally posted by @drammock in https://github.com/pydata/pydata-sphinx-theme/issues/1445#issuecomment-1877861639

xia-stan commented 10 months ago

Clarifications will go a long way to help with this feature. I've been banging my head against it for about a day. I started digging into the JS and realized that the version switcher expects semver compliant versions. My project doesn't always use semver compliant names, which was making the whole thing act funny.

FranzRoters commented 9 months ago

I have a comment and maybe a suggestion on this topic.

Indeed looking at the sphinx documentation, using release for checking the banner to show makes perfect sense.

I assume that many people, like initially myself, believe that release should refer to the last "released" version in contrast to a "development" version. Now, this last "released" version is what you show as x.y.z (stable) in your version switcher. However, in switcher.json you use the preferred flag to indicate that same version. Wouldn't it make sense to use the same name for both? I would suggest use of stable, as that is what this version should be, while "preferred" is a matter of personal choice.

Adidtional question, does the preferred flag also initialize a substitution |preferred| to reference that version in the docs?

drammock commented 9 months ago

Indeed looking at the sphinx documentation, using release for checking the banner to show makes perfect sense.

glad you agree :)

I assume that many people, like initially myself, believe that release should refer to the last "released" version in contrast to a "development" version.

The meaning of those variables is determined by Sphinx (not by theme authors), and is described in their docs. Briefly: version is expected (by Sphinx) to be X.Y, and release is expected to be X.Y.Z or possibly X.Y.Zrc1 or X.Y.Zdev0 or similar. Typically the conf.py (where those vars are defined) is version-controlled along with the repo, so sometimes release will be X.Y.Z and correspond to a "stable" or version of the software and its corresponding "stable" documentation. In between stable releases, the value of release can (and in some cases arguably should) change to X.Y.Zdev0 or X.Y.Zrc1 or similar. Moreover: neither of those variables as defined in conf.py are necessarily used for version matching with respect to the version switcher JSON. That is done by html_theme_options["switcher"]["version_match"] which might (but need not!) be set to the same value as version or release.

Now, this last "released" version is what you show as x.y.z (stable) in your version switcher. However, in switcher.json you use the preferred flag to indicate that same version. Wouldn't it make sense to use the same name for both? I would suggest use of stable, as that is what this version should be, while "preferred" is a matter of personal choice.

I am struggling to follow exactly what the suggested change is here. Is it a change to the guidance in our docs? Or a change to the key names we use in html_theme_options? Or a change to how we name the fields in the JSON file? Or a change to the particular values we specify in the JSON file for our own docs?

Adidtional question, does the preferred flag also initialize a substitution |preferred| to reference that version in the docs?

No it does not. It's a neat idea, but not super easy, because which version is preferred is defined in the .json file. Sphinx substitutions like |preferred| are typically defined in the .rst documents themselves, in .inc files that are included into some/all of the rST documents, or as part of the rst_prolog variable in conf.py. Substitutions |version| and |release| are special cases that are built into Sphinx, which creates them automatically from the corresponding python variables of the same name. To do the same thing for preferred for a single project, you would need to add code to your conf.py that:

  1. parses the JSON file given in `html_theme_options["switcher"]["json_url"]
  2. finds the preferred flag on one of the items in the JSON
  3. finds the corresponding version attribute of that item
  4. adds something like f".. |preferred| {preferred_version}\n" to the rst_prolog variable.

it's probably also possible to do something similar in the theme's __init__ file but there the process is less clear to me off the top of my head.

FranzRoters commented 9 months ago

I assume that many people, like initially myself, believe that release should refer to the last "released" version in contrast to a "development" version.

The meaning of those variables is determined by Sphinx (not by theme authors), and is described in their docs. Briefly: version is expected (by Sphinx) to be X.Y, and release is expected to be X.Y.Z or possibly X.Y.Zrc1 or X.Y.Zdev0 or similar. Typically the conf.py (where those vars are defined) is version-controlled along with the repo, so sometimes release will be X.Y.Z and correspond to a "stable" or version of the software and its corresponding "stable" documentation. In between stable releases, the value of release can (and in some cases arguably should) change to X.Y.Zdev0 or X.Y.Zrc1 or similar.

I believe this is exactly the point, where the confusion of many users starts. What should be called a release? E.g. in X.Y.Zrc1 the "rc" means "release candidate", so many people would argue that it is actually not a release but a version that should evantually become a release. Similar for "dev". On the other hand, any version that is made publically available can be defined as being released, i.e. being a release. Sphinx obviously follows the second definition.

Moreover: neither of those variables as defined in conf.py are necessarily used for version matching with respect to the version switcher JSON. That is done by html_theme_options["switcher"]["version_match"] which might (but need not!) be set to the same value as version or release.

I understand this, but as mentioned above release is used for determining whether and which banner to show, and I believe that's where many people are confused (see above).

Now, this last "released" version is what you show as x.y.z (stable) in your version switcher. However, in switcher.json you use the preferred flag to indicate that same version. Wouldn't it make sense to use the same name for both? I would suggest use of stable, as that is what this version should be, while "preferred" is a matter of personal choice.

I am struggling to follow exactly what the suggested change is here. Is it a change to the guidance in our docs? Or a change to the key names we use in html_theme_options? Or a change to how we name the fields in the JSON file? Or a change to the particular values we specify in the JSON file for our own docs?

Here I intended to suggest to change the key name in the JSON file to "stable".

Adidtional question, does the preferred flag also initialize a substitution |preferred| to reference that version in the docs?

No it does not. It's a neat idea, but not super easy, because which version is preferred is defined in the .json file. Sphinx substitutions like |preferred| are typically defined in the .rst documents themselves, in .inc files that are included into some/all of the rST documents, or as part of the rst_prolog variable in conf.py. Substitutions |version| and |release| are special cases that are built into Sphinx, which creates them automatically from the corresponding python variables of the same name. To do the same thing for preferred for a single project, you would need to add code to your conf.py that:

1. parses the JSON file given in `html_theme_options["switcher"]["json_url"]

2. finds the `preferred` flag on one of the items in the JSON

3. finds the corresponding `version` attribute of that item

4. adds something like `f".. |preferred| {preferred_version}\n"` to the `rst_prolog` variable.

it's probably also possible to do something similar in the theme's __init__ file but there the process is less clear to me off the top of my head.

As you see I am not so much in the details of writing themes and this indeed seems more complicated than anticipated. So for now ite seems easier to manually edit the rst_prolog in line with switcher.json.

drammock commented 9 months ago

The meaning of those variables is determined by Sphinx (not by theme authors) [...]

I believe this is exactly the point, where the confusion of many users starts.

Correct me if I'm wrong, but I think this means that the most we could do here is "make our own docs a bit clearer". I'm happy to try to do that / review other's PRs that do so. We do already link to the Sphinx definitions in the switcher-version-match section but maybe we should also do so in the warning banner section?

Moreover: neither of those variables as defined in conf.py are necessarily used for version matching with respect to the version switcher JSON. That is done by html_theme_options["switcher"]["version_match"] which might (but need not!) be set to the same value as version or release.

I understand this, but as mentioned above release is used for determining whether and which banner to show, and I believe that's where many people are confused (see above).

Ah, point taken --- I had lost track of the fact that this was about the warning banner too and not just the version switcher. So I guess the warning banner section needs to make clear that it is the release variable that is used for the warning banner's version matching.

I am struggling to follow exactly what the suggested change is here. [...]

Here I intended to suggest to change the key name in the JSON file to "stable".

I'm not sure I agree with this proposal. "preferred" is more generic, which I think is more accomodating / less confusing for certain cases. In particular, projects that maintain multiple "stable" versions (think of the era of python 2.x and 3.x both being supported, for example), or cases where the developers want users to always be reading the "dev" version of the docs (I've encountered this at least once, but I forget where).

I think I can envision now some concrete improvements to the docs. A couple are fairly straightforward and mentioned above:

  1. link to Sphinx definitions of "stable" and "release" from the warning-banner section
  2. add to the warning-banner section the fact that "release" is what is used for checking against

Additionally, I think it's probably worthwhile to add some background/explanation about why it works this way. Namely:

That last bullet reveals another non-docs thing we could do that might (?) make things easier: change the JSON field name from version to release --- at least then it would be clearer (?) that the release field in JSON is compared to release in conf.py (via the intermediary of DOCUMENTATION_OPTIONS.VERSION) when deciding if this page warrants a warning banner or not. But, as you've pointed out @FranzRoters, people's prior assumptions about what "release" means is one source of interference here, so I'm not sure that doubling down on the use of that term (by changing the JSON field name to release) will actually be an improvement. (not to mention the code churn needed while everyone updates their JSON files). So I'm leaning pretty strongly toward a docs-only "fix" here. Curious to hear your thoughts after reading all this.

FranzRoters commented 9 months ago

Indeed I did not think about cases with multiple stable versions.

So, I do agree that improoving the documentation is the best thing to do. As you said, once the users understands how the existing scheme works, he should be able to work with what is there. In case really needed, one can still define additional varibales in conf.py.

xia-stan commented 9 months ago

Explaining in the documentation that we have to define both the version and release variables would be helpful. Sphinx doesn't list either of those fields as required. You can remove any of the Project Configuration variables from conf.py and still get a "successful" build. It's an example of variable defaults at its best and worst. I don't need to ever set the values, but if downstream tools depend on them, I'll get behavior that I don't expect.

PyData Sphinx Theme introduces functionality tied to variables that, when initialized to their defaults, causes unexpected behavior. Maybe a two prong approach? Update the documentation and add a utility function that validates version and release that issues a warning if they're not set? I'm not super familiar with the inner workings of the Sphinx build process, so I don't know how difficult the latter might be. The documentation update could be as simple as stating that both those variables are required fields in the conf.py for compatibility with this theme.