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

[doc] Add a CI check for the doc being properly generated #6736

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented May 29, 2022

Type of Changes

Type
✨ New feature
📜 Docs

Description

Refs #5953

Refs #6735 (comment)

Require #8949, #8950, #8951, #8956

@Pierre-Sassoulas Pierre-Sassoulas added Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow labels May 29, 2022
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.github/workflows/checks.yaml Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@coveralls
Copy link

coveralls commented May 29, 2022

Pull Request Test Coverage Report for Build 3400448677

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.38%

Totals Coverage Status
Change from base Build 3400152690: 0.0%
Covered Lines: 17239
Relevant Lines: 18074

💛 - Coveralls

@github-actions

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Member Author

Task failed successfully. I think we need a better error message because it's very unclear that the issue is that the file were modified during the make. Ideally pre-commit could autofix, but I don't know how well that work with the "manual" option.

@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as draft May 29, 2022 13:47
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the pre-commit-check-doc-generation branch 2 times, most recently from 93ea4a3 to b37f1fd Compare May 29, 2022 21:49
@github-actions

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented May 29, 2022

I think that adding this hook as pre-push is reasonable. The alternative I think would be to add it all the time but only if some specific file are modified like checkers in pylint/checkers and python file in doc/exts/.

Con/pro of the pre-push hook:

  • ➖ No automatic fix
  • ➖ Need to install pre-commit with command that is not well known imo
  • ➕ Not launched for every commit locally

Con/pro of the pre-commithook:

  • ➕ Automatic fix online
  • ➖ It's very slow, we're going to --no-verify a lot of commits if we don't do something about it

@github-actions

This comment has been minimized.

@DanielNoord
Copy link
Collaborator

I'm still not sure about adding this as a local pre-commit hook. I worry that I would start using --no-verify quite a bit more. For me creating the documentation takes 30+ seconds while also spinning up all fans my laptop can find 😅 When I'm working on the primer for example this is something I would like to avoid.

On pydocstringformatter I created a workflow that builds the documentation and then checks if there is a diff after that build. If there is we fail the workflow. Would that be okay here as well?

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the pre-commit-check-doc-generation branch 2 times, most recently from f945d38 to cc2b93a Compare May 31, 2022 11:12
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as ready for review May 31, 2022 11:13
@Pierre-Sassoulas
Copy link
Member Author

@DanielNoord @jacobtylerwalls sorry I changed the contributor doc a little more than strictly necessary but it was in such a state of disrepair that I could not help it.

@Pierre-Sassoulas
Copy link
Member Author

Let it be noted that the tests fail successfully before the generation of the doc on cc2b93a

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the pre-commit-check-doc-generation branch from fc0a932 to 7bad7f7 Compare May 31, 2022 13:26
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 98e684b

@Pierre-Sassoulas Pierre-Sassoulas removed the Needs decision 🔒 Needs a decision before implemention or rejection label Aug 13, 2023
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Aug 13, 2023

I chose to hard code the value in a dict, the generic way to do it was not worth it as we have so little dynamically generated values.

@Pierre-Sassoulas
Copy link
Member Author

Sorry for the ping on this one, I'm going to create 2 new merge requests to not mix things up (#8949 and #8950).

@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit ea000d9

@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 8edc45c

@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit c08cfc4

DanielNoord
DanielNoord previously approved these changes Aug 13, 2023
And cleanup the doc about dev installation for contributors.

Co-authored-by: Jacob Walls <[email protected]>
@Pierre-Sassoulas Pierre-Sassoulas removed the Blocked 🚧 Blocked by a particular issue label Aug 15, 2023
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as ready for review August 15, 2023 14:08
Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I'm thinking that it could be nice to launch the fast python generation directly in a pre-commit hook but I want to prioritize the conf upgrade tool atm

@Pierre-Sassoulas Pierre-Sassoulas merged commit f40e9ff into pylint-dev:main Aug 15, 2023
33 checks passed
@Pierre-Sassoulas Pierre-Sassoulas deleted the pre-commit-check-doc-generation branch August 15, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants