Switch to `sphinx-theme-builder` and improve webpack.config.js #514

pradyunsg · 2021-11-11T18:22:56Z

Closes #503.

This is going to be easiest to review commit-by-commit, where each commit has a clear description of why the changes were made.

pradyunsg · 2021-11-11T20:49:32Z

Hehe, I goofed up Python 3.7 support.

choldgraf · 2021-11-13T16:30:45Z

It seems like the linting changes are gonna create a big diff...maybe we could do that one as a separate PR so that it is easier for others to review the theme builder update?

pradyunsg · 2021-11-13T16:43:59Z

Either way, this PR is going to be easiest to review commit-by-commit.

pradyunsg · 2021-11-23T07:52:46Z

\o/

The documentation build works! https://pydata-sphinx-theme--514.org.readthedocs.build/en/514/

The interpreter goes EoL in ~1 month, and it is not supported by `sphinx-theme-builder`.

Restructure the repository to be compatible with `sphinx-theme-builder` and switch away from setuptools. This enables excluding the generated assets from version control, since `sphinx-theme-builder` will generate them, when generating the wheel. Replace the usage of `yarn` with vanilla `npm`, to be more compatible with `sphinx-theme-builder`, and for reducing the complexity in the setup. Explicitly declare that this theme uses the latest 14.x LTS release of NodeJS. Simplify `noxfile.py` since it is no longer necessary to use conda for fetching the theme's non-Python dependencies. It happens transparently in a `pip` based workflow. Use `stb serve` for serving documentation, in `noxfile.py`. Move `flake8` configuration into a flake8-specific file.

Realistically, users should be using sphinx-build directly OR use the nox targets to generate the documentation.

This will hopefully save someone a Google search.

Utilise a dedent helper, and include the hash as a digest query parameter.

pradyunsg · 2021-11-23T07:59:11Z

Hehe, codecov is complaining because I removed lines, reducing the coverage %age. 😆

pradyunsg · 2021-11-23T08:04:10Z

Most of the changes here now, are either:

related to the Webpack configuration cleanup.
related to setuptools -> sphinx-theme-builder switch.
- a file moved without changes.
- a compiled file was removed from version control.
- a new configuration file.

pradyunsg · 2021-11-23T08:12:00Z

Here's a quick link to the "main" files that were changed.

astrojuanlu · 2021-11-23T09:28:21Z

pradyunsg · 2021-11-23T21:29:26Z

😅

jorisvandenbossche

@pradyunsg thanks a lot for this effort! This is really great

Tried it out locally, easy to set up and use, everything went smooth (except I first forgot to install the package again before calling stb serve)

Added a few inline comments/questions. And two additional things:

The "Publish to PyPI" will still need to be updated I suppose? (we are using, ahum, setup.py dist bdist_wheel .. 🤦) That should be a simple stb package ?
The contributing.rst will need some more updates (but I am also happy to take a look at this in a follow-up)

docs/Makefile

jorisvandenbossche · 2021-11-24T09:43:26Z

noxfile.py

-        "-n", "-b", "html", "docs/", "docs/_build/html"
-    )
-    # fmt: on
+    session.install("sphinx-theme-builder[cli]")


Does this also need a session.install(".[doc]")?

I think that the theme builder will automatically handle the dependencies of the local theme so we don't need to explicitly install it here, but maybe @pradyunsg can confirm that?

It does need that line. I missed it.

Yep, I can confirm this is needed but I guess we can fix it in a follow-up PR.

noxfile.py

jorisvandenbossche · 2021-11-29T21:57:09Z

@damianavila @choldgraf any comments / concerns here, or shall I go ahead and merge this (and update the other PRs)

damianavila · 2021-11-29T22:19:40Z

.github/workflows/tests.yml


  # Build docs on a number of Python versions. In the future this can be
  # where tests go.
  tests:
    runs-on: ubuntu-latest
    strategy:
      matrix:
-        python-version: [3.6, 3.7, 3.8, 3.9]
+        python-version: [3.7, 3.8, 3.9]


I guess we should also add 3.10 now?

IMO we should add that explicitly in a separate PR, because we don't know if this might break something. I feel more comfortable about dropping supported versions as a side-effect, rather than adding them :-)

Fair enough ;-)

noxfile.py

docs/contributing.rst

choldgraf

This looks good to me, provided that there are no objections to the outstanding comments. I think it's a much cleaner workflow once we get the hang of it, and I am thrilled that we will no longer have merge conflicts for the hashes every time somebody merges some CSS/JS :-)

Next steps

How about this plan:

Leave this open for a day to see if @damianavila has any comments he think should block merging for now.
If not, or @damianavila has ideas but that don't need to block a merge, then we just merge this one in and iterate in subsequent PRs.

choldgraf · 2021-11-29T22:18:17Z

.github/workflows/tests.yml


  # Build docs on a number of Python versions. In the future this can be
  # where tests go.
  tests:
    runs-on: ubuntu-latest
    strategy:
      matrix:
-        python-version: [3.6, 3.7, 3.8, 3.9]
+        python-version: [3.7, 3.8, 3.9]


I just want to highlight that we're dropping Python 3.6 support here, in case anybody has a strong objection. I think it is fine because it will no longer have official support soon anyway, but just making it clear here.

choldgraf · 2021-11-29T22:20:12Z

noxfile.py

-        "-n", "-b", "html", "docs/", "docs/_build/html"
-    )
-    # fmt: on
+    session.install("sphinx-theme-builder[cli]")


I think that the theme builder will automatically handle the dependencies of the local theme so we don't need to explicitly install it here, but maybe @pradyunsg can confirm that?

noxfile.py

damianavila · 2021-11-29T22:36:13Z

I just left some tiny comments 😉 !
I think several of the questions raised by @jorisvandenbossche here should be addressed before merging it.
Finally, I still need to deeply test this one and I really want to understand a little bit of the builder internals before providing a more profound review. So I would probably request for at least 48 hs (given that I will be driving 9 hs tomorrow and I may be a little bit tired to provide a sensible second review 😉 ).

jorisvandenbossche · 2021-11-30T11:56:37Z

We can certainly leave it open for a day longer if that helps (and much appreciated you want to test drive this thoroughly, @damianavila!). But I think most feedback we (will) have here is good as follow-up PRs anyway.
The comments are mostly about updating documentation, ensuring nox workflows still work as expected, maybe feedback to sphinx-theme-builder to improve documentation or fix certain bugs/features. And not about the core changes in this PR required to adopt sphinx-theme-builder (eg the noxfile changes are not essential to this, and might be easier to discuss separately).

Merging this PR sooner rather than later / addressing those comments in follow-ups will make it easier to actually test those new workflows by using them while working on the theme. The noxfile is also still quite new anyway, and will have to settle down a bit (I am also not sure what eg the benefit of nox -s docs-live is now we also have stb serve docs)

choldgraf · 2021-11-30T14:48:05Z

Given that @jorisvandenbossche is a conda person and still thinks that the workflow here is reasonable (potentially could be documented more cleanly, but we can do that in follow up PRs), I'd be +1 on merging after 24 hours so that we can move forward. For remaining review here, I think we should focus on major changes that might need to be made, but handle smaller details, docs, etc in follow-ups after we've had a chance to work with it.

damianavila · 2021-12-02T16:54:17Z

Update:
I will finish my deep-dive and testing in a few hours.
Things are looking good so far... if continues in that direction, I will approve it by EOD.
Thanks for your patience!

damianavila

OK, everyone, I think now I have a clear understanding of what is happening here 😉
I have tested the PR successfully except for the docs-live issue referenced above that can be fixed in a follow-up PR.

It is interesting to notice how layered we would be from now on the JS side of things (and from the development point of view):

nox
stb
npm

We would need some docs updates on nox and new stuff for the stb.
Wondering if it makes sense to go deeper (or not) since we are aiming at "python devs that need JS stuff"... but we can talk about that later.

LGTM to merge this one as is 😉 and iterate on follow-ups.

choldgraf · 2021-12-03T00:21:09Z

wohoo! I'll merge this one in now! @jorisvandenbossche wanna give a rebase in #436 ?

pradyunsg · 2021-12-03T08:45:55Z

I'm seeing a few follow ups here. Does someone else want to file issues to track that they get done?

jorisvandenbossche · 2021-12-03T09:54:38Z

I think the follow-ups are:

Update the "Publish to PyPI" action (we are using, ahum, setup.py dist bdist_wheel .. 🤦) That should be a simple stb package ?
Add session.install(".[doc]") to nox (Switch to sphinx-theme-builder and improve webpack.config.js #514 (comment))
The contributing.rst will need some more updates
Not directly related to this, but add Python 3.10 to test matrix

For the first + last one I will directly do a PR, and will open an issue for the docs.

jorisvandenbossche · 2021-12-03T12:41:15Z

And thanks a lot @pradyunsg once more! This is a wonderful tooling improvement!

You can see more details in the following comment: #514 (comment)

More details are available here: pydata#514 (comment)

damianavila · 2021-12-03T21:31:06Z

For the first + last one I will directly do a PR, and will open an issue for the docs.

I think that was resolved in #519 although I had some question there after the PR was merged.

Add session.install(".[doc]") to nox (Switch to sphinx-theme-builder and improve webpack.config.js #514 (comment))

I quickly opened a PR for this one: #520.

The contributing.rst will need some more updates

I also captured this one in a new issue: #521.

pradyunsg changed the title ~~Switch to sphinx-theme-builder~~ More pre-commit linting and switch to sphinx-theme-builder Nov 12, 2021

jorisvandenbossche mentioned this pull request Nov 22, 2021

Add more pre-commit linting #518

Merged

pradyunsg force-pushed the switch-to-sphinx-theme-builder branch from ac502f7 to 1f5bcc9 Compare November 23, 2021 07:43

pradyunsg changed the title ~~More pre-commit linting and switch to sphinx-theme-builder~~ Switch to sphinx-theme-builder and improve webpack.config.js Nov 23, 2021

pradyunsg force-pushed the switch-to-sphinx-theme-builder branch 2 times, most recently from de0dc70 to 731d673 Compare November 23, 2021 07:48

pradyunsg added 7 commits November 23, 2021 07:57

Drop Python 3.6 support

b530c3a

The interpreter goes EoL in ~1 month, and it is not supported by `sphinx-theme-builder`.

Drop make stuff from docs/

6156c8a

Realistically, users should be using sphinx-build directly OR use the nox targets to generate the documentation.

Add a comment about what ?-url means

5ffa841

This will hopefully save someone a Google search.

Enable source map for the generated JS file

c3bdb3c

Improve webpack-macros.html generation logic

8427e3c

Utilise a dedent helper, and include the hash as a digest query parameter.

Simplify linting setup

bd6168b

pradyunsg force-pushed the switch-to-sphinx-theme-builder branch from 364e940 to bd6168b Compare November 23, 2021 07:59

pradyunsg marked this pull request as ready for review November 23, 2021 08:00

pradyunsg mentioned this pull request Nov 23, 2021

Stop bundling the compiled CSS/JS with our theme by following Furo's build pattern #503

Closed

2 tasks

jorisvandenbossche reviewed Nov 24, 2021

View reviewed changes

This was referenced Nov 24, 2021

Note pip > 21.3 is required for an editable install pradyunsg/sphinx-theme-builder#4

Merged

✨ NEW: add version switcher #436

Merged

damianavila reviewed Nov 29, 2021

View reviewed changes

noxfile.py Show resolved Hide resolved

damianavila reviewed Nov 29, 2021

View reviewed changes

docs/contributing.rst Show resolved Hide resolved

choldgraf approved these changes Nov 29, 2021

View reviewed changes

damianavila approved these changes Dec 2, 2021

View reviewed changes

choldgraf merged commit 102f741 into pydata:master Dec 3, 2021

pradyunsg deleted the switch-to-sphinx-theme-builder branch December 3, 2021 08:44

jorisvandenbossche mentioned this pull request Dec 3, 2021

CI: update github action workflow after sphinx-theme-builder switch, remove Python 3.6 #519

Merged

damianavila added a commit that referenced this pull request Dec 3, 2021

Add step to install needed dependencies to build the site

afd4ee8

You can see more details in the following comment: #514 (comment)

damianavila added a commit to damianavila/pydata-sphinx-theme that referenced this pull request Dec 3, 2021

Add step to install needed dependencies to build the site

c7dc69f

More details are available here: pydata#514 (comment)

This was referenced Dec 3, 2021

Add step to install needed dependencies to build the site damianavila/pydata-sphinx-theme#1

Closed

Add step to install needed dependencies to build the site #520

Merged

jorisvandenbossche mentioned this pull request Dec 6, 2021

Contributor page needs updates after landing stb-related improvements #521

Closed

damianavila mentioned this pull request Dec 6, 2021

Team Sync - Monday, December 6th 2i2c-org/team-compass#324

Closed

drammock mentioned this pull request Dec 28, 2021

Define a release policy #532

Closed

choldgraf mentioned this pull request Jan 10, 2022

DEV: workflow with the fingerprinting / hash in index.css/js filenames #241

Closed

jorisvandenbossche mentioned this pull request Jan 10, 2022

DOCS: refactor developer docs #538

Merged

This was referenced Jan 14, 2022

ENH: Sphinx-basic-ng as the base instead of pydata-sphinx-theme executablebooks/sphinx-book-theme#456

Closed

Switch our build system to use the sphinx-theme-builder executablebooks/sphinx-book-theme#417

Closed

choldgraf mentioned this pull request May 17, 2022

ENH: simplify color variables #659

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to `sphinx-theme-builder` and improve webpack.config.js #514

Switch to `sphinx-theme-builder` and improve webpack.config.js #514

pradyunsg commented Nov 11, 2021 •

edited

Loading

pradyunsg commented Nov 11, 2021

choldgraf commented Nov 13, 2021

pradyunsg commented Nov 13, 2021

pradyunsg commented Nov 23, 2021

pradyunsg commented Nov 23, 2021

pradyunsg commented Nov 23, 2021 •

edited

Loading

pradyunsg commented Nov 23, 2021 •

edited

Loading

astrojuanlu commented Nov 23, 2021

pradyunsg commented Nov 23, 2021

jorisvandenbossche left a comment

jorisvandenbossche Nov 24, 2021

choldgraf Nov 29, 2021

pradyunsg Dec 2, 2021 •

edited

Loading

damianavila Dec 2, 2021

jorisvandenbossche commented Nov 29, 2021

damianavila Nov 29, 2021

choldgraf Nov 29, 2021

damianavila Nov 29, 2021

choldgraf left a comment

choldgraf Nov 29, 2021

choldgraf Nov 29, 2021

damianavila commented Nov 29, 2021 •

edited

Loading

jorisvandenbossche commented Nov 30, 2021 •

edited

Loading

choldgraf commented Nov 30, 2021 •

edited

Loading

damianavila commented Dec 2, 2021

damianavila left a comment

choldgraf commented Dec 3, 2021

pradyunsg commented Dec 3, 2021

jorisvandenbossche commented Dec 3, 2021

jorisvandenbossche commented Dec 3, 2021

damianavila commented Dec 3, 2021

Switch to sphinx-theme-builder and improve webpack.config.js #514

Switch to sphinx-theme-builder and improve webpack.config.js #514

Conversation

pradyunsg commented Nov 11, 2021 • edited Loading

pradyunsg commented Nov 11, 2021

choldgraf commented Nov 13, 2021

pradyunsg commented Nov 13, 2021

pradyunsg commented Nov 23, 2021

pradyunsg commented Nov 23, 2021

pradyunsg commented Nov 23, 2021 • edited Loading

pradyunsg commented Nov 23, 2021 • edited Loading

astrojuanlu commented Nov 23, 2021

pradyunsg commented Nov 23, 2021

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Nov 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choldgraf left a comment

Choose a reason for hiding this comment

Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianavila commented Nov 29, 2021 • edited Loading

jorisvandenbossche commented Nov 30, 2021 • edited Loading

choldgraf commented Nov 30, 2021 • edited Loading

damianavila commented Dec 2, 2021

damianavila left a comment

Choose a reason for hiding this comment

choldgraf commented Dec 3, 2021

pradyunsg commented Dec 3, 2021

jorisvandenbossche commented Dec 3, 2021

jorisvandenbossche commented Dec 3, 2021

damianavila commented Dec 3, 2021

Switch to `sphinx-theme-builder` and improve webpack.config.js #514

Switch to `sphinx-theme-builder` and improve webpack.config.js #514

pradyunsg commented Nov 11, 2021 •

edited

Loading

pradyunsg commented Nov 23, 2021 •

edited

Loading

pradyunsg commented Nov 23, 2021 •

edited

Loading

pradyunsg Dec 2, 2021 •

edited

Loading

damianavila commented Nov 29, 2021 •

edited

Loading

jorisvandenbossche commented Nov 30, 2021 •

edited

Loading

choldgraf commented Nov 30, 2021 •

edited

Loading