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

docs: add warning when rendering preview docs #2559

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SnoopJ
Copy link
Contributor

@SnoopJ SnoopJ commented Nov 12, 2023

Description

I had not realized that the unstable documentation was being regularly built against master, and I thought it would be helpful if these preview docs included a warning about their preview-ness as well as a reference to the specific commit they were built against.

The warning added by this changeset is included on all pages using Sphinx's rst_prolog variable, but only rendered when the Sopel version is a pre-release (i.e. .devN) version.

Below is an example of how preview documentation renders (the links points to the relevant commit on GitHub and https://sopel.chat/docs/, respectively)

image

And here's what the docs look like for a release version:

image

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
  • I have tested the functionality of the things this change touches

Comment on lines 320 to 322
is_dirty = bool(subprocess.check_output(["git", "status", "--untracked-files=no", "--porcelain"], text=True).strip())
commit_hash = subprocess.check_output(["git", "rev-parse", "HEAD"], text=True).strip()
git_describe = subprocess.check_output(["git", "describe", "--tags"], text=True).strip() + ("-dirty" if is_dirty else "")
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 have been very much enjoying the versioningit project over the last year or so, which can automate away this sort of stuff (at the expense of reading the version from a _version.py file or somesuch instead of using importlib metadata). But I tried to write this patch using only what's already present, and it should work fine as long as git is present and the docs are being built from the repository, and degrade gracefully if there are problems (i.e. you'll still get a warning box if git is not present or the CWD violates this code's assumptions)

The output of `git-describe` is in terms of the latest tag on the
current branch, which can produce surprising results when tags are
distributed across release branches (i.e. `v7.1.0-992-g51300a1a` is the
best it can produce for that commit, even though the latest stable tag
at that point is `v7.1.9`).

It makes more sense to refer only to the commit.
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

At the moment I'm only commenting because I don't know if I like the result more than I hate the process: using subprocess in Python is something I'm very averse to for many reasons.

@SnoopJ
Copy link
Contributor Author

SnoopJ commented Nov 13, 2023

using subprocess in Python is something I'm very averse to for many reasons.

Is that a generalized aesthetic concern, or are you thinking of specific failure modes here?

An alternative that occurred to me while putting this PR together would be to add a Makefile target for the preview documentation that retrieves the relevant commit information and passes it to Sphinx. I don't know what the specific mechanism would be, but I'm sure we could pass it in.

Edit: per discussion on IRC, I will rewrite to shove as much git wrangling into the Makefile as possible, without error handling if git is not present or if calling git fails.

@SnoopJ SnoopJ force-pushed the doc/preview-docs-warning-header branch from b86c872 to 4009450 Compare November 14, 2023 00:26
Comment on lines +328 to +329
if Version(__version__).is_prerelease or commit_hash:
tags.add("preview")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: with the change to an environment variable, the preview tag is added if the user set the environment variable, even if the __version__ is not a pre-release. It seems prudent that the output of make docs_preview would always set the tag

commit_hash = os.environ.get("SOPEL_GIT_COMMIT", "")[:7]
is_dirty = bool(os.environ.get("SOPEL_GIT_DIRTY"))
if commit_hash:
github_ref = "https://github.com/sopel-irc/sopel/commit/{commit_hash}".format(commit_hash=commit_hash)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It occurred to me while preparing my revisions that this GitHub link isn't guaranteed to be valid when building the preview docs against commits that aren't pushed to the repo, but I'm not sure that's worth worrying about, it should be valid in general for work we do in this repo (and the docs that end up on the website), and anybody hacking on a local clone of the source probably will understand if the link is invalid.

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.

None yet

2 participants