♻️ REFACTOR: Asset compilation and dev workflow #211

chrisjsewell · 2020-09-22T18:50:26Z

This refactor follows development of the workflow in https://github.com/executablebooks/sphinx-panels and https://github.com/executablebooks/web-compile.
It fixes a number of pressing issues (particularly see jupyter-book/jupyter-book#921):

The package is using a SCSS compiler (pyScss) which is not well maintained and has some important un-adressed issues (e.g. it can't handle compiling CSS custom variables)
Currently we are writing CSS files into the installed folder (not good at all!)
The CSS/JS is not hashed, and so when people update their sphinx-book-theme, they may not always get the up-to-date files (due to caching)

The approach here is essentially very similar to pydata-sphinx-theme. But instead of using webpack to compile the SCSS/JS we are using the web-compile pre-commit hook. This limits the development complexity of having to work with an additional nodejs environment, and allows everything to run via tox.

As previously discussed, both of these approaches (including the compiled/hashed files in the repo) do suffer one drawback: that merge conflicts can arise if not making PRs to the latest commit on the master branch.
However, this use-case has literally never come up yet, so I'd rather accept this con for now, in return for the more important fixes/pros.
Note, there is talk of eventually doing this compilation at build time (see executablebooks/meta#137), but I'm not satisfied yet that there is an ideal solution (and in any case this is a step in the direction).

As discussed previously I moved from nox to tox, since tox is (a) the more mature/used package and (b) used across other EBP packages.

The tox -e docs-live, in my testing, look to be working well, and now correctly updates as you change the source Python/SCSS/JS.

The contributing guide has also been updated to reflect these changes: https://sphinx-book-theme--211.org.readthedocs.build/en/211/contributing.html

codecov · 2020-09-22T18:52:58Z

Codecov Report

Merging #211 into master will decrease coverage by 0.44%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
- Coverage   92.11%   91.66%   -0.45%     
==========================================
  Files           2        1       -1     
  Lines         241       72     -169     
==========================================
- Hits          222       66     -156     
+ Misses         19        6      -13

Flag	Coverage Δ
#pytests	`91.66% <ø> (-0.45%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a53df7...a4c929b. Read the comment docs.

chrisjsewell · 2020-09-22T18:55:29Z

The contributing guide has also been updated to reflect these changes: https://sphinx-book-theme--211.org.readthedocs.build/en/211/contributing.html

docs/conf.py

choldgraf · 2020-09-22T19:47:23Z

setup.py

    },
    entry_points={"sphinx.html_themes": ["sphinx_book_theme = sphinx_book_theme"]},
-    package_data={


this is unnecessary because we have the MANIFEST file right?

choldgraf · 2020-09-22T19:48:29Z

docs/contributing.md


-```bash
-nox -s docs-live
+### Working on the theme


can we document theme_dev_mode

yeh, as mentioned in the code comments, this option should literally only be used in this repos conf.py; to allow for the CSS/JS to be dynamically updated

choldgraf · 2020-09-22T19:51:27Z

web-compile-config.yml

+      src/js/sphinx-book-theme.js: sphinx_book_theme/static/sphinx-book-theme.[hash].js
+  jinja:
+    files:
+      src/jinja/theme.conf.j2: sphinx_book_theme/theme.conf


so this pre-commit hook will generate the new CSS and JS files, + a new theme.conf file if the previous files have changed at all?

yep, give it a go! thats the best way to see how (if 🤞) it works

and obviously you can have a look at https://github.com/executablebooks/web-compile if you want to understand further

choldgraf · 2020-09-22T20:12:06Z

hmmm, when I ran tox -e py38-sphinx3, it didn't seem to install most of the dependencies and then failed on myst_parser is not installed. Is there an extra command we're missing in the dev docs for installing the environment w/ tox?

Also, because I don't have Python 3.7 installed just running tox yields this error:

ERROR: InterpreterNotFound: python3.7

chrisjsewell · 2020-09-22T20:14:32Z

have you previously used tox in this folder, i.e. did a .tox folder already exist?
If so, have you tried either running: tox -r -e py38-sphinx3, or just deleting the folder.
If not, what is the console output?

choldgraf · 2020-09-22T20:16:32Z

first time using tox, here's the output of the command you suggested:

py38-sphinx3 create: /home/choldgraf/github/forks/python/ebp/sphinx-book-theme/.tox/py38-sphinx3
py38-sphinx3 installdeps: sphinx>=3,<4
py38-sphinx3 develop-inst: /home/choldgraf/github/forks/python/ebp/sphinx-book-theme
py38-sphinx3 installed: alabaster==0.7.12,attrs==20.2.0,Babel==2.8.0,beautifulsoup4==4.9.1,certifi==2020.6.20,chardet==3.0.4,click==7.1.2,coverage==5.3,docutils==0.16,idna==2.10,imagesize==1.2.0,iniconfig==1.0.1,Jinja2==2.11.2,MarkupSafe==1.1.1,more-itertools==8.5.0,packaging==20.4,pluggy==0.13.1,py==1.9.0,pydata-sphinx-theme==0.4.0,Pygments==2.7.1,pyparsing==2.4.7,pytest==6.0.2,pytest-cov==2.10.1,pytest-datadir==1.3.1,pytest-regressions==2.0.1,pytz==2020.1,PyYAML==5.3.1,requests==2.24.0,six==1.15.0,snowballstemmer==2.0.0,soupsieve==2.0.1,Sphinx==3.2.1,-e git+https://github.com/choldgraf/sphinx-book-theme@e28bb5ce3d18dc8c3d00011ac224fd3894524301#egg=sphinx_book_theme,sphinxcontrib-applehelp==1.0.2,sphinxcontrib-devhelp==1.0.2,sphinxcontrib-htmlhelp==1.0.3,sphinxcontrib-jsmath==1.0.1,sphinxcontrib-qthelp==1.0.3,sphinxcontrib-serializinghtml==1.1.4,toml==0.10.1,urllib3==1.25.10
py38-sphinx3 run-test-pre: PYTHONHASHSEED='4177624336'

note that I don't see myst_parser etc being installed though it does install the theme

chrisjsewell · 2020-09-22T20:17:37Z

Also, because I don't have Python 3.7 installed just running tox yields this error:

If you are using conda (?) you can just create an environment like conda create -n python=3.7, thentox-conda will be able to find it.

If not yeh you just have to run with the specified environment. You can set for tox to run all available python versions. But I've personally found that more hassle than its worth.

choldgraf · 2020-09-22T20:21:17Z

hmmm, I installed toc-conda and while tox now does run, it has the same error as the manual tox command I showed above.

chrisjsewell · 2020-09-22T20:28:05Z

failed on myst_parser is not installed

Ah well thats because its currently only set to install the testing extras requirements, and myst-parer is not part is not in that. Do we really need to install the whole sphinx extras to run the tests?

chrisjsewell · 2020-09-22T20:30:15Z

but basically just replace extras = testing with

extras =
  testing
  sphinx

choldgraf · 2020-09-22T20:30:37Z

well we do have a notebook and myst markdown files in the tests, so we'd at least need those two unless we want to re-write the tests to only use rST files

chrisjsewell · 2020-09-22T20:33:22Z

Yeh but you don't really need to waste time installing the large packages like numpy, pandas, matplotlib, etc.
But anyway just adding the sphinx extra should fix it?

choldgraf · 2020-09-22T20:40:55Z

yep adding sphinx got it working. I also tried adding myst_nb and sphinx_thebe to the tests requirements, and that got it working too...maybe that is the easiest path forward?

chrisjsewell · 2020-09-22T20:42:09Z

yep thats what I was just doing 👍 Obviously the idea with these dev environments is that they should only contain what is actually necessary to perform the command

choldgraf · 2020-09-22T20:44:58Z

cool, makes sense to me too, probably just cruft from the old days.

The pre-commit workflow worked for me 👍 once you get this merged I can give it a shot with a fix for jupyter-book/jupyter-book#986 (comment)

setup.py

This refactor follows development of the workflow in https://github.com/executablebooks/sphinx-panels and https://github.com/executablebooks/web-compile. It fixes a number of pressing issues (particularly see jupyter-book/jupyter-book#921): - The package is using a SCSS compiler (pyScss) which is not well maintained and has some important un-adressed issues (e.g. it can't handle compiling CSS custom variables) - Currently we are writing CSS files into the installed folder (not good at all!) - The CSS/JS is not hashed, and so when people update their sphinx-book-theme, they may not always get the up-to-date files (due to caching) The approach here is essentially very similar to pydata-sphinx-theme. But instead of using webpack to compile the SCSS/JS we are using the web-compile pre-commit hook. This limits the development complexity of having to work with an additional nodejs environment, and allows everything to run *via* tox. As previously discussed, both of these approaches (including the compiled/hashed files in the repo) do suffer one drawback: that merge conflicts can arise if not making PRs to the latest commit on the `master` branch. However, this use-case has literally never come up yet, so I'd rather accept this con for now, in return for the more important fixes/pros. Note, there is talk of eventually doing this compilation at build time (see executablebooks/meta#137), but I'm not satisfied yet that there is an ideal solution (and in any case this is a step in the direction). As discussed previously I moved from nox to tox, since tox is (a) the more mature/used package and (b) used across other EBP packages.

Also add Windows testing

closes #206 Note, Path.absolute is undocumented and faulty so should not be used.

chrisjsewell requested review from pradyunsg and choldgraf September 22, 2020 18:51

choldgraf reviewed Sep 22, 2020

View reviewed changes

docs/conf.py Outdated Show resolved Hide resolved

choldgraf reviewed Sep 22, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

chrisjsewell force-pushed the fix-asset-builds branch from 4942964 to b6d186e Compare September 22, 2020 22:58

chrisjsewell added 3 commits September 23, 2020 00:00

🧪 TESTS: Use sphinx test fixtures

1a55f27

Also add Windows testing

🐛 FIX: Replace absolute/resolve methods with os.path.abspath

a4c929b

closes #206 Note, Path.absolute is undocumented and faulty so should not be used.

chrisjsewell force-pushed the fix-asset-builds branch from b6d186e to a4c929b Compare September 22, 2020 23:01

chrisjsewell merged commit e4b33e8 into master Sep 22, 2020

chrisjsewell deleted the fix-asset-builds branch September 22, 2020 23:06

jorisvandenbossche mentioned this pull request Jan 24, 2021

TST: refactor to use sphinx test fixtures pydata/pydata-sphinx-theme#308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ REFACTOR: Asset compilation and dev workflow #211

♻️ REFACTOR: Asset compilation and dev workflow #211

chrisjsewell commented Sep 22, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading

chrisjsewell commented Sep 22, 2020

choldgraf Sep 22, 2020

chrisjsewell Sep 22, 2020

choldgraf Sep 22, 2020

chrisjsewell Sep 22, 2020

choldgraf Sep 22, 2020

chrisjsewell Sep 22, 2020

chrisjsewell Sep 22, 2020

choldgraf commented Sep 22, 2020 •

edited

Loading

chrisjsewell commented Sep 22, 2020

choldgraf commented Sep 22, 2020 •

edited

Loading

chrisjsewell commented Sep 22, 2020

choldgraf commented Sep 22, 2020

chrisjsewell commented Sep 22, 2020 •

edited

Loading

chrisjsewell commented Sep 22, 2020

choldgraf commented Sep 22, 2020

chrisjsewell commented Sep 22, 2020 •

edited

Loading

choldgraf commented Sep 22, 2020

chrisjsewell commented Sep 22, 2020

choldgraf commented Sep 22, 2020

♻️ REFACTOR: Asset compilation and dev workflow #211

♻️ REFACTOR: Asset compilation and dev workflow #211

Conversation

chrisjsewell commented Sep 22, 2020 • edited Loading

codecov bot commented Sep 22, 2020 • edited Loading

Codecov Report

chrisjsewell commented Sep 22, 2020

choldgraf Sep 22, 2020

Choose a reason for hiding this comment

chrisjsewell Sep 22, 2020

Choose a reason for hiding this comment

choldgraf Sep 22, 2020

Choose a reason for hiding this comment

chrisjsewell Sep 22, 2020

Choose a reason for hiding this comment

choldgraf Sep 22, 2020

Choose a reason for hiding this comment

chrisjsewell Sep 22, 2020

Choose a reason for hiding this comment

chrisjsewell Sep 22, 2020

Choose a reason for hiding this comment

choldgraf commented Sep 22, 2020 • edited Loading

chrisjsewell commented Sep 22, 2020

choldgraf commented Sep 22, 2020 • edited Loading

chrisjsewell commented Sep 22, 2020

choldgraf commented Sep 22, 2020

chrisjsewell commented Sep 22, 2020 • edited Loading

chrisjsewell commented Sep 22, 2020

choldgraf commented Sep 22, 2020

chrisjsewell commented Sep 22, 2020 • edited Loading

choldgraf commented Sep 22, 2020

chrisjsewell commented Sep 22, 2020

choldgraf commented Sep 22, 2020

chrisjsewell commented Sep 22, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading

choldgraf commented Sep 22, 2020 •

edited

Loading

choldgraf commented Sep 22, 2020 •

edited

Loading

chrisjsewell commented Sep 22, 2020 •

edited

Loading

chrisjsewell commented Sep 22, 2020 •

edited

Loading