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

Conditional removal of changelogs #4943

Merged
merged 6 commits into from
Sep 18, 2024
Merged

Conversation

kb-0311
Copy link
Contributor

@kb-0311 kb-0311 commented Sep 17, 2024

Fixes

Fixes #4726 by @sarayourfriend

Description

Based on an environment variable conditionally removes the changelogs

Testing Instructions

Checklist

  • My pull request has a descriptive title (not a vague title likeUpdate index.md).
  • My pull request targets the default branch of the repository (main) or a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.
  • I ran the DAG documentation generator (ov just catalog/generate-docs for catalog
    PRs) or the media properties generator (ov just catalog/generate-docs media-props
    for the catalog or ov just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@kb-0311 kb-0311 requested a review from a team as a code owner September 17, 2024 08:04
@kb-0311 kb-0311 requested review from AetherUnbound and sarayourfriend and removed request for a team September 17, 2024 08:04
@openverse-bot openverse-bot added 🧱 stack: documentation Related to Sphinx documentation 🟩 priority: low Low priority and doesn't need to be rushed 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🤖 aspect: dx Concerns developers' experience with the codebase labels Sep 17, 2024
@kb-0311
Copy link
Contributor Author

kb-0311 commented Sep 17, 2024

@sarayourfriend I am not sure if this is the right approach for this issue.

Also right now I am having trouble adding further commits here, could not find what could we do here?:

kb-0311@kb0311-IdeaPad-5-Pro-14ITL6:~/Desktop/openverse$ git commit -m "lint"
`pre-commit` not found.  Did you forget to activate your virtualenv?

@sarayourfriend
Copy link
Collaborator

@kb-0311 Can you confirm you have you set up ov for linting? It comes pre-loaded with pre-commit so shouldn't have that issue. The output of cat .git/hooks/pre-commit should be this, if you have:

#! /usr/bin/env bash

export TERM=xterm

./ov hook pre-commit "$@"

If not, please follow the ov setup instructions on this documentation page, starting with ./ov init from the repository root, and then committing again.

Let me know if you run into any issues with setting up ov.

@kb-0311
Copy link
Contributor Author

kb-0311 commented Sep 17, 2024

Thanks a lot , fixed the env setup error : )

Coming to how the redirects are being handled here in my PR:

if "changelogs" in exclude_patterns:
# temporary placeholder for now
redirects["changelogs/"] = (
"/general/contribution/github_contribution_practices.html"
)

I am not really sure if this is the right way to go about it. Also, I could not really find the "Page not found" file location so just added a temporary value for now.

@sarayourfriend
Copy link
Collaborator

This LGTM! I have two small requests, and then this will be good to go from my perspective.

First is to add the following to documentation/conf.py so that we don't see non-actionable warnings in local builds:

nitpick_ignore_regex = () if INCLUDE_CHANGELOGS else (
    ("myst", ".*/changelogs/index.*"),
)

This uses the Sphinx configuration option nitpick_ignore_regex to ignore "nitpick" warnings about the invalid cross-reference which is the only targeted method to ignore this error that MyST's documentation describes.


The other thing to do is address the redirect. I like the idea of using a redirect, and it simplifies things a lot to do so. However, it would be nice to redirect to a page that explains why the changelogs are missing, if someone happens to run across a link to the changelogs that doesn't resolve when they are working in docs locally. Here is a diff you can work from that does that, by adding an orphan page under meta to redirect to when INCLUDE_CHANGELOGS is false:

diff --git a/documentation/conf.py b/documentation/conf.py
index 86035b925..a4d5e33ac 100644
--- a/documentation/conf.py
+++ b/documentation/conf.py
@@ -137,9 +137,6 @@ redirects = {
 }
 
 if "changelogs" in exclude_patterns:
-    # temporary placeholder for now
-    redirects["changelogs/"] = (
-        "/general/contribution/github_contribution_practices.html"
-    )
+    redirects["changelogs/index"] = "/meta/missing_changelogs.html"
 
 myst_enable_extensions = ["linkify"]
diff --git a/documentation/meta/missing_changelogs.md b/documentation/meta/missing_changelogs.md
new file mode 100644
index 000000000..c98635e87
--- /dev/null
+++ b/documentation/meta/missing_changelogs.md
@@ -0,0 +1,13 @@
+---
+orphan: true
+---
+
+# Changelogs missing from local build
+
+The documentation build excludes changelogs unless it is in a CI environment or explicitly instructed to include changelogs. This significantly reduces the number of files processed for local preview builds, which cuts startup time of the documentation preview by half and improving the ergonomics of working with preview documentation locally.
+
+To force the inclusion of the changelogs, run the documentation build with `INCLUDE_CHANGELOGS=1` in your environment. For example, the following will run the preview build, including changelogs:
+
+```shell
+ov env INCLUDE_CHANGELOGS=1 just documentation/serve
+```

Let me know if these requests make sense, or if you would like help implementing them, and thanks as always for the contribution @kb-0311 🙏

Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

I meant to add that feedback to a request changes comment, sorry for the double ping.

@kb-0311
Copy link
Contributor Author

kb-0311 commented Sep 18, 2024

@sarayourfriend Added the suggestions as well in the new commit, please do take a look when you are free.

And thanks a lot for the help : )

Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

LGTM! I was able to test this locally by building with CI=1 and seeing the changelogs show up still and everything works perfectly. This is such a significant improvement, thanks again @kb-0311 🚀

@sarayourfriend sarayourfriend merged commit 6e9b893 into WordPress:main Sep 18, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: documentation Related to Sphinx documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Sphinx/MYsT dev build is very slow
3 participants