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

add pre-commit-hook configuration to this repo #2932

Closed
wants to merge 2 commits into from

Conversation

Cube707
Copy link

@Cube707 Cube707 commented Aug 14, 2022

I don't know if this feature interest you, so its a draft for now. I also didn't add documentation for now unitll I have feedback if this is desirable:

I implemented a pre-commit hook for this repository (see her for more info). This allows users to easiely integrate the mkdocs build command into their workflow. It will run on any commit that has changes to relevant files (mkdocs.yml or any markdown file insode docs/) and will stop the commit if the build process fails.

It also makes it easy to have invalid PRs fail the ci-workflow with a single simple workflow, instead of having to create a extra step manually to run mkdocs.

All a user needs to to is to install pre-commit (most likly already the case) and add this to his pre-commit-config.yaml:

  - repo: https://github.com/Cube707/mkdocs  # only for temporary testing
    # repo: https://github.com/mkdocs/mkdocs  # this will be the real URL 
    rev: 878df16  # will be a github-Tag marking a release
    hooks:
      - id: mkdocs-build

if this feature is accabtabel, this is to be discussed:

  • add documentation (where would be the appropriate place? maybe docs/deploying-your-docs.md#pre-commit?)
  • once merged, do we want to add this to the list on precommit.com?

@oprypin
Copy link
Contributor

oprypin commented Aug 16, 2022

My first reaction is against it :/
How does it handle if there are any external plugins used?

@Cube707
Copy link
Author

Cube707 commented Aug 16, 2022

How does it handle if there are any external plugins used?

good question. I didn't think of that. I will try and test

@Cube707
Copy link
Author

Cube707 commented Aug 16, 2022

The user can simply define additional_dependencies, which accepts a list of packages installable via pip.

Here is an example:

  - repo: https://github.com/Cube707/mkdocs
    rev: 878df16
    hooks:
      - id: mkdocs-build
        additional_dependencies:
          - mkdocs-plugin-progress

Co-authored-by: Álvaro Mondéjar <mondejar1994@gmail.com>
@davidxia
Copy link

davidxia commented Dec 1, 2022

Just wanted to say this would be very helpful for me. 🙏

@davidxia
Copy link

davidxia commented Dec 1, 2022

@oprypin does @Cube707's latest comments address your concern?

In general lmk if there's any next steps I can help with.

@pawamoy
Copy link
Contributor

pawamoy commented Apr 30, 2023

Can files be configured by users? (I have almost zero experience with pre-commit)
This is needed because the config file is not always mkdocs.yml and the docs are not always stored in a docs folder.
(sorry, misclicked on close with comment...)

@pawamoy pawamoy closed this Apr 30, 2023
@pawamoy pawamoy reopened this Apr 30, 2023
@Cube707
Copy link
Author

Cube707 commented Apr 30, 2023

@pawamoy yes, in fact all keys can be overriden by the user if needed.

The .pre-commit-hook.yml file is just a set of sensible default values making it run out of the box most of the time and beeing a referenc on how to interact with this repo in an automated manner for all other cases.

require_serial: true
pass_filenames: false
types_or: [markdown, yaml]
files: ((^|/)mkdocs\.ya?ml$)|(^docs/)
Copy link
Contributor

@mondeja mondeja Apr 30, 2023

Choose a reason for hiding this comment

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

I think that the files field should be just removed as said previously you can change the docs directory and configuration file path.

Copy link
Contributor

Choose a reason for hiding this comment

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

But based on what @Cube707 explained, this value is just a default value which makes sense in most cases. Users can change it in their projects if needed.

Copy link
Author

@Cube707 Cube707 May 4, 2023

Choose a reason for hiding this comment

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

yes what @pawamoy said. But also the files field is responsible for limiting what files trigger a mkdoc build and if removed it will run for changes to any file. As mkdoc is quite expencive to run it should defnetly be limited in when it runs.

An argument could be made here to match no files at all by default, making it that it must be run manually (via pre-commit run mkdocs-build) or the user must provide/overide the file patterns

@pawamoy
Copy link
Contributor

pawamoy commented Apr 30, 2023

Thanks for the explanation @Cube707. I wonder something though: is that reasonable from pre-commit to ask third-party projects to host this configuration themselves? Isn't there some kind of central repository for such tool configurations? It's a bit unfair to put the maintenance burden on third-party tools which might not be interested in pre-commit at all, even if that maintenance is very low. Just asking for myself: I have projects that could be used through pre-commit, but I wouldn't like having to maintain a .pre-commit-hooks.yaml in the root of my repository.

@Cube707
Copy link
Author

Cube707 commented May 4, 2023

no, you are defnetly only supposed to host a hooks file on a project if you explicitly choose to support pre-commit.

There are options for a pre-commit-user to interact with a tool that has decided againts supporting pre-commit directly. (like I said, the hooks file is just a set of defaults, if its not there, all pre-commit users have to figure out these settings by themselfs and might do it incorectly)

having the hooks file as part of a project makes it much more likely that it gets updated should the tools change in a way that requires an updated and avoids breaking downstream users CI-pipelines.

But there are mirrows of bigger repos that add the .pre-commit-hooks.yaml, for example the autopep8 mirror. Some of them maintained by pre-commit themselfs and some maintained by the community.


So bottom line: we must decide here if pre-commit support is desired or not and if not, the PR will be closed and the .pre-commit-hooks.yaml must be mainanted elsewhere (on a mirrow of some sort)

@mkusz
Copy link

mkusz commented Oct 16, 2023

Pre commits are to validate and fix some errors in the code and from one perspective, this could be useful if you thread docs as part of the code. On the other hand, docs/ files are documentation and files in site/ are only the output of the build process (the same as a release files). Because of that, those files should be considered as a build artifacts and generation process should be pushed for CI/CD (like GitHub Actions).

For me, a useful pre-commit hook would be the one that runs the build process, and block commit when any WARN/ERROR/CRITICAL is seen in an output when strict mode is enabled.

@Cube707 Cube707 closed this by deleting the head repository Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants