refactor: spring cleaning #1271

12rambau · 2023-03-27T22:41:03Z

As mentioned in the title this PR is not about functional changes to pydata-sphinx-theme but only some clean up of our files. Let me do a list of the modifications:

clean pre-commits
- Drop flake8 in favor of ruff. As We are testing everything I had to add a lot of docstring in packages and modules and change the format of some to respect Google style. Nothing else was required to make it work. It's integrated to our pre-commits.
- drop all the pre-commits linting small hooks as they are all integrated to prettier
move the gallery_directive to an _extention folder as it's not a script that should be launched. Updated the conf.py file accordingly.
clean the noxfile file:
- remove name when it's not necessary. By design it uses the name of the method, only use the name option when dealing with "_"
- update the pybabel session to make it launchable without arguments. as we should be able to run nox without any other arguments to run all the sessions
- create a lint session to apply pre-commits
- the compile session was not doing anything anymore, I set back the compilation command
clean the tests
- move the build factory feature to the conftest.py file
- apply ruff to all test sites (docstring mainly)
clean ini.py. As mentioned in sanityse __init__.py #1216 the file was becoming longer and longer with artificial splits made out of comments. To me it's usualy the sign that the file deserve to be split. I hope that it will improve readability and help newcomer understand what we are doing in one look:
- move the setup_translators application function to the translator.py
- create an utils.py file with the common config parameters helpers
- move all toctree related methods to a toctree.py file.
- keep in the __init__.py the update_config with all the deprecation notices and the layout creation
everywhere I passed, started to add hints in hope that one day we could run mypy on the lib
edit the github workflow to create a matrix and fast-fail when a job fails. check the Action page to see the links: https://github.com/pydata/pydata-sphinx-theme/actions/runs/4540443615

choldgraf

I likely don't have bandwidth to give this an extremely detailed review, but I am a big +1 on the general approach, esp splitting __init__.py into modules, and I think if the docs look the same and the tests still pass, I'm +1 on merging unless somebody finds something obviously wrong. We can always iterate and these refactors will help us maintain and keep track of stuff much better, I think.

drammock

Overall +1 on this. I did a quick look and left a few comments. Please though let's wait for #1264 to merge first

drammock · 2023-03-28T14:30:41Z

src/pydata_sphinx_theme/__init__.py

+from .edit_this_page import setup_edit_url
+from .logo import copy_logo_images, setup_logo_path
+from .pygment import overwrite_pygments_css
+from .short_link import ShortenLinkTransform
+from .toctree import add_toctree_functions
+from .translator import setup_translators
+from .utils import config_provided_by_user, get_theme_options


WDYT about putting these in a directory with an __init__? So this would become, e.g.,

from ._helpers import setup_edit_url, copy_logo_images, setup_logo_path, etc

IDK what the best name is (probably doesn't matter much). Ideas: _tools, _pst, _helpers, _init_helpers, etc

I'm not a big fan of creating a extra folder to store them but I see you point of reducing the number of import lines. Let me try something

what do you think of the modifications made in 71f73c2 ? It avoids the extra folder and greatly simplify the imort statements.

good enough for me. Personally I prefer having in a folder (when I ls a directory shorter lists are easier for me to scan quickly) but it's a matter of taste so I won't push the issue.

docs/_extention/gallery_directive.py

src/pydata_sphinx_theme/toctree.py

12rambau · 2023-03-28T15:12:08Z

@drammock a new file was generated by bootstrap: pydata-sphinx-theme/src/pydata_sphinx_theme/theme/pydata_sphinx_theme/static/scripts/bootstrap.js.LICENSE.txt. I think it's related to #1258 should I add it to gitignore ?

drammock · 2023-03-28T15:52:46Z

I don't see it in the diff of #1258, and I don't think we changed the bootstrap version in that PR 🤷🏻

In any event it's possible that we're supposed to include the license if we redistribute the source; can you peek at the license and see if that's required?

12rambau · 2023-03-29T21:29:05Z

I cheked the bootstrap file and now the `bootstrap.js file starts with

/*! For license information please see bootstrap.js.LICENSE.txt */

So I assumed it should be generted and added it to the ignored files.

drammock

This looks good @12rambau. I'll go ahead and commit the spelling / docstring fixes I suggested; then there's one question I'll let you decide whether to do it now or leave it alone. Feel free to merge once you make that call.

docs/_extension/gallery_directive.py

drammock · 2023-03-29T22:16:24Z

src/pydata_sphinx_theme/short_link.py

+def traverse_or_findall(node: Node, condition: str, **kwargs) -> Iterator[Node]:
+    """Triage node.traverse (docutils <0.18.1) vs node.findall.
+
+    TODO: This check can be removed when the minimum supported docutils version
+    for numpydoc is docutils>=0.18.1.
+    """
+    return (
+        node.findall(condition, **kwargs)
+        if hasattr(node, "findall")
+        else node.traverse(condition, **kwargs)
+    )


IMO this would make more sense in .utils (or perhaps .fixes or .shims?) But since I think it's only used this one place (right?) I guess let's leave it alone.

@12rambau this is the one I want your second opinion about, whether it's worth it to move it to .utils (or some other file) now.

it's true it could be reused, let me move it to .utils

drammock · 2023-03-29T22:17:50Z

src/pydata_sphinx_theme/toctree.py

oof, this file is a beast :) just commenting to say I didn't really read this file / I'm trusting that it's a copy/paste from __init__

no worries, it's a copy/paste. Edit anything there without caution and you'll break the sidebars

tests/test_build.py

src/pydata_sphinx_theme/pygment.py

choldgraf

I took a look at the PR diff and it looks good to me - I also looked through the example docs and it looks good as well. So I'm +1 on merging this and cutting out a patch release!

12rambau added 6 commits March 27, 2023 22:30

sanitize babel calls

369c0ee

move the gallery directive to an extention folder

5d12c59

clean test folder

8a87cba

split __init__.py

7ce98e5

add links between tests

8e44258

drop flake8 from pyproject.toml

dd2c05a

12rambau marked this pull request as ready for review March 28, 2023 07:33

choldgraf reviewed Mar 28, 2023

View reviewed changes

drammock reviewed Mar 28, 2023

View reviewed changes

12rambau added 2 commits March 28, 2023 17:06

rename folder

e95d9c3

update docstring

5122bc9

drammock mentioned this pull request Mar 28, 2023

Override generation of id & href attributes for API documentation #1208

Merged

12rambau added 2 commits March 29, 2023 15:12

only import modules instead of functions

71f73c2

ignore bootstrap license

55bcff9

12rambau and others added 2 commits March 29, 2023 23:35

Merge branch 'main' into ruff

54452d9

reintegrate modifications from pydata#1264

f0b3205

drammock mentioned this pull request Mar 29, 2023

v0.13.2 is incompatible with sphinx-book-theme #1274

Closed

drammock approved these changes Mar 29, 2023

View reviewed changes

drammock and others added 3 commits March 29, 2023 17:31

typos / docstring improvements / whitespace

0d68647

fix: expose traverse_or_findall in utils

326f97f

Merge branch 'ruff' of github.com:12rambau/pydata-sphinx-theme into ruff

67fdff6

choldgraf approved these changes Mar 30, 2023

View reviewed changes

12rambau merged commit 0b58fca into pydata:main Mar 30, 2023

12rambau deleted the ruff branch March 30, 2023 14:30

12rambau mentioned this pull request Apr 13, 2023

can we simplify how our tests are defined and run? #1292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: spring cleaning #1271

refactor: spring cleaning #1271

12rambau commented Mar 27, 2023 •

edited

Loading

choldgraf left a comment

drammock left a comment

drammock Mar 28, 2023

12rambau Mar 29, 2023

12rambau Mar 29, 2023

drammock Mar 29, 2023

12rambau commented Mar 28, 2023

drammock commented Mar 28, 2023

12rambau commented Mar 29, 2023

drammock left a comment

drammock Mar 29, 2023

drammock Mar 29, 2023

12rambau Mar 30, 2023

drammock Mar 29, 2023

12rambau Mar 30, 2023 •

edited

Loading

choldgraf left a comment

refactor: spring cleaning #1271

refactor: spring cleaning #1271

Conversation

12rambau commented Mar 27, 2023 • edited Loading

choldgraf left a comment

Choose a reason for hiding this comment

drammock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

12rambau commented Mar 28, 2023

drammock commented Mar 28, 2023

12rambau commented Mar 29, 2023

drammock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

12rambau Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

choldgraf left a comment

Choose a reason for hiding this comment

12rambau commented Mar 27, 2023 •

edited

Loading

12rambau Mar 30, 2023 •

edited

Loading