Allow relative json url to work locally (and hopefully on PRs)

[damianavila]

Additionally, this is fixing a bug recently introduced and described in #574 (comment)

[choldgraf]

This looks good to me, with one caveat:

Historically we have not had a strong practice of using dev / dev0 etc releases for the theme. E.g., our release docs don't mention it, they just suggest bumping the version just before releasing, and leaving the version as-is until the next release. Another option would be to check for os.environ["READTHEDOCS"] instead of checking for dev in the version. I don't have strong opinions on it, though "remember to add dev0 is "yet another thing to remember" when making releases and requires an extra commit...if we have docs that have a strong assumption about the presence of "dev0", then we'll need to make this more like a formal policy

[tupui]

Oh it's great to have a way to test locally! If it works I think it should be documented. This is something all projects will want to do.

[jorisvandenbossche]

Checking this locally, the dropdown now works (no console error + the shorter path for json_url works locally to directly see the effect of changing the json file).
But, it seems that while the error is gone, the actual extra_classes does not work yet. At least, I don't see any "dev" or "stable" classes being added to the button or list item.

---

Historically we have not had a strong practice of using dev / dev0 etc releases for the theme. E.g., our release docs don't mention it, they just suggest bumping the version just before releasing, and leaving the version as-is until the next release.

Yeah, that's true. We did it a few times in the past (eg a293862), and so therefore I did it in #574 so we could properly detect the version for the dropdown (but that was done before you added the os.environ["READTHEDOCS"] check)

Another option would be to check for os.environ["READTHEDOCS"] instead of checking for dev in the version.

That would indeed be sufficient for the online docs. But not for testing it locally, I think?

I don't have strong opinions on it, though "remember to add dev0 is "yet another thing to remember" when making releases and requires an extra commit...if we have docs that have a strong assumption about the presence of "dev0", then we'll need to make this more like a formal policy

Personally, I would rather use something like versioneer or setuptools-scm that simply takes care of this for us, and we only have to tag for making a release (because I agree that having to do such a "bump to dev" commit manually after a release is just extra work that is easy to forget).
But I don't know if that would be compatible with sphinx-theme-builder (from a quick look, it seems that it expects the version to be either in pyproject.toml, or be directly assigned to __version__ = .. in __init__.py

[jorisvandenbossche]

But, it seems that while the error is gone, the actual extra_classes does not work yet. At least, I don't see any "dev" or "stable" classes being added to the button or list item.

Sorry, was looking at the wrong place .. It is actually adding it to the version_switcher_menu id, but I think that's the wrong place to add it (the comment in the code is also saying it is adding extra classes to the "button", so this probably should have been version_switcher_button). I think #597 is also already fixing this.

[damianavila]

What about using the jupyter releaser to automate the whole release process so we do not forget any steps. Btw, that project suggest tbump to bump versions.

[choldgraf]

Interesting - the jupyter releases looks quite powerful, though the getting starts docs seem pretty hefty. maybe as a start we could just try using tbump ? That way the release process would be:

1. Run tbump
2. Make a release on GitHub associated with the tag

I've found it useful to have the step of adding a GitHub release anyway, so that you can link to it on Twitter etc.

That said, I still don't find it that cumbersome to just bump the version by hand to the next version, and not use dev0 etc in general. I haven't used the .dev tag on repositories for many months now and haven't run into any negative issues with it.

Also just to clarify re: os.environ["READTHEDOCS"]:

That would indeed be sufficient for the online docs. But not for testing it locally, I think?

I think the conditional would be something like:

if os.environ.get("READTHEDOCS"):
  json_url = "remote-url"
else:
  json_url = "local-path"

wouldn't that effectively do the same as the dev/remote workflow described here? (at least for 99% of the use-cases)

[damianavila]

maybe as a start we could just try using tbump ?

That's a good first step, I would say.

That said, I still don't find it that cumbersome to just bump the version by hand to the next version

I would agree as well, maybe we just need to add the step in the release checklist for now?

wouldn't that effectively do the same as the dev/remote workflow described here? (at least for 99% of the use-cases)

I don't think so... if you are in RTD (in a PR preview) and pointing to the "remote-url", you will have a cross-origin error preventing the dropdown from actually working as expected. This PR is fixing that case pointing to the "local-url" when you are working locally AND when you are in a PR preview.

[choldgraf]

let's merge this one in - I think there's agreement on the fix even if we are following up in another issue