Skip to content

Conversation

@OliverSieweke
Copy link

@OliverSieweke OliverSieweke commented Apr 26, 2020

Feature or Bugfix

  • Feature

Purpose

This change allows to easily configure sphinx as a git hook over the pre-commit framework, so as to:

  • Ensure the documentation is building for each commit
  • Automatically keep the local documentation up to date

Detail

To use sphinx as a git hook over pre-commit with this change, you may follow the steps below:

  1. Install pre-commit in you project
pip install pre-commit
pre-commit install
  1. Create an appropriate .pre-commit-config.yaml file at your project root. Example:
repos:
  - repo: https://github.com/sphinx-doc/sphinx
    rev: 3.x
    hooks:
      - id: docs
        args: [-W, ./docs/source, ./docs/build]
        additional_dependencies:
          - sphinx_rtd_theme
        always_run: true
        files: ".^"

NB 1: The files and always_run keys are set so that the build runs exactly once without matching any files.
NB 2: My fork can be used for testing by setting repo: https://github.com/OliverSieweke/sphinx

  1. Commit and see the hook running

Below are examples of passing and failing documentation builds:

sphinx-valid-commit
sphinx-invalid-commit

References:

This change allows to easily configure sphinx as a git hook over the
pre-commit framework, so as to:
- Ensure the documentation is building for each commit
- Automatically keep the local documentation up to date

References:
- https://pre-commit.com/
- https://pre-commit.com/#new-hooks

Example for a .pre-commit-config.yaml configuration:

repos:
  - repo: https://github.com/sphinx-doc/sphinx
    rev: 3.x
    hooks:
      - id: docs
        args: [-W, ./docs/source, ./docs/build]
        additional_dependencies:
          - sphinx_rtd_theme
        always_run: true
        files: ".^"
@tk0miya
Copy link
Member

tk0miya commented Apr 26, 2020

-1: most of the commits are not related to changes with the document. And we've already build our document on the CI service. So I feel this is too much.

@tk0miya tk0miya added type:proposal a feature suggestion type:tests labels Apr 26, 2020
@OliverSieweke
Copy link
Author

Thanks for the feedback @tk0miya.
Just to clarify: the aim of this proposal is not to set up a hook for sphinx’s own documentation. Rather it gives developers using sphinx the possibility to set it up as a git hook for their own project using pre-commit, which seems to be the most popular git hook framework in the python ecosystem. (pre-commit requires the .pre-commit-hooks.yaml file to be present in the repo to work).

I agree that this may not be a fitting tool for every project and may represent an unnecessary overhead for non-documentation related commits on large projects (although I was under the impression that sphinx makes use of caching before deciding whether to rebuild part of the docs).

I thought it wouldn't come at any harm and might offer quite handy solutions in particular for small projects using sphinx with autodoc for instance.

@tk0miya
Copy link
Member

tk0miya commented Apr 27, 2020

If my understanding is correct, what you should do is add a tip page to the Sphinx document. I suppose users of Sphinx does not read the dotfile on the repository. I think this PR effects to developers of Sphinx only.

@OliverSieweke
Copy link
Author

Well, this addition is meant for the general user, but you’re right that he would probably not be aware of this possibili.y without looking at the dotfiles.

If you’re keen, I would be happy to further submit an update to the documentation along with the change. I had a look at the contents and haven’t found an existing section that would fit. It might be worth adding an Integrations section that could also include documentation on integration with other tools. I am thinking something along those lines:

Sphinx documentation contents
  ├── Introduction
  ├── Installing Sphinx
  ┆
  ├── Integrations
        ├── Git Hooks
        	├── pre-commit
	├── Editor Integrations
	├── Read the Docs Integration

Note that pre-commit further maintains a list of repositories with supported hooks, to which Sphinx could be added in case this pull request gets accepted/

@jakobandersen
Copy link
Contributor

As @tk0miya mentioned, the location of the file is not good and could adversely affect those that clone the Sphinx repo. How about adding it as an option to sphinx-quickstart, similar to how it asks about Makefile generation?

@tk0miya
Copy link
Member

tk0miya commented Apr 29, 2020

Okay, I understand how pre-commit works. It just reads .pre-commit-hooks.yaml from this repository and invoke sphinx-build command to build their own document, right?
If so, +1 for adding the recipe and document.

BTW, is pre-commit able to change the command by the version of the installed package? We have a plan to change the form of sphinx-build command (maybe in the future, refs: #6938). At that time, we will change the recipe script to the new one. But I worried the change will break the users who uses pre-commit and old Sphinx. Is there any idea?

@tk0miya
Copy link
Member

tk0miya commented Apr 29, 2020

It might be worth adding an Integrations section that could also include documentation on integration with other tools.

LGTM. At first, I thought it is better to add it to the FAQ page https://www.sphinx-doc.org/en/master/faq.html. But I prefer your idea.

@stephenfin
Copy link
Contributor

I'm definitely a fan of this idea and would love to see it. However, we need docs. I'd suggest putting it into docs/usage/advanced/pre-commit.rst. We need an example of how one could use this in their project and why they'd want to do so. Also, configuration. I assume we'd need a way to configure the build and source directories, at a minimum.

@OliverSieweke
Copy link
Author

I have been quite busy lately, but I am happy to take care of the documentation for this. Will propose something in the next 2 weeks.

@tk0miya concerning changes to the sphinx-build command, the hook is always configured for a specific git tag/ref, so updates should not break existing user configurations.

@stephenfin thanks for the feedback. Not entirely sure what your concerns were with the configurations, but my understanding is that the .pre-commit-hooks.yaml in the sphinx repo sets up the hook and provides sensible defaults, which can then partly be overwritten by the pre-commit-config.yaml in the user's repo.

@stephenfin
Copy link
Contributor

@stephenfin thanks for the feedback. Not entirely sure what your concerns were with the configurations, but my understanding is that the .pre-commit-hooks.yaml in the sphinx repo sets up the hook and provides sensible defaults, which can then partly be overwritten by the pre-commit-config.yaml in the user's repo.

I have only used existing pre-commit hooks and not written my own, so apologies if I'm misunderstanding things (feel free to link docs proving me wrong), however, you can't just call sphinx-build without augment. Rather, you need at a minimum a path to the source directory and a path to the output directory (e.g. sphinx-build SRC_DIR OUT_DIR). I haven't yet groked how you'd configure these via your .pre-commit.yaml`. Surely you need parameters?

@OliverSieweke
Copy link
Author

Yes you're right. You can easily set all those parameters in your project's .pre-commit.yaml.
For example, those are the configs I used when I tested things out on my fork (my specific SOURCE_DIR and OUT_DIR are provided in the args field):

repos:
  - repo: https://github.com/sphinx-doc/sphinx
    rev: 3.x
    hooks:
      - id: docs
        args: [-W, ./docs/source, ./docs/build]
        additional_dependencies:
          - sphinx_rtd_theme
        always_run: true
        files: ".^"

The upcoming documentation should be able to clarify all this...

@tk0miya
Copy link
Member

tk0miya commented Jan 23, 2021

I'm closing this because of not updated long. In my understanding, a new documentation entry is needed to merge this. Please resend a new one if it will be ready for review.
#7558 (comment)

Thanks,

@tk0miya tk0miya closed this Jan 23, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

type:proposal a feature suggestion type:tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants