Skip to content

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Aug 5, 2025

Contributes to #6949 and #6901, by moving some logic out of test.sh

  • moves rstcheck into pre-commit
  • puts doxygen in the docs-build conda environment so we don't need to manually install it in scripts

Notes for Reviewers

How I tested this

Added a directive like this to a file in docs/:

.. garbage::

Then ran pre-commit

pre-commit run --all-files

Saw it catch that

rstcheck.................................................................Failed
- hook id: rstcheck
- exit code: 1

An `AttributeError` error occured. This is most probably due to a code block directive (code/code-block/sourcecode) without a specified language. This may result in a false negative for source: 'docs/Installation-Guide.rst'. The reason can also be another directive. For more information see the FAQ (https://rstcheck-core.rtfd.io/en/latest/faq) or the corresponding github issue: https://github.com/rstcheck/rstcheck-core/issues/3.
docs/Advanced-Topics.rst:49: (ERROR/3) Unknown directive type "garbage".
Error! Issues detected.

@jameslamb jameslamb changed the title WIP: [ci] use 'pre-commit' to run 'rstcheck' [ci] use 'pre-commit' to run 'rstcheck' Aug 5, 2025
@jameslamb jameslamb marked this pull request as ready for review August 5, 2025 02:25
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM!
Could you please rename .ci/lint-python-bash.sh to something like .ci/run-pre-commit-mypy.sh because pre-commit becoming more than just python linting jobs?

@jameslamb
Copy link
Collaborator Author

LGTM! Could you please rename .ci/lint-python-bash.sh to something like .ci/run-pre-commit-mypy.sh because pre-commit becoming more than just python linting jobs?

Sure, good point! Just did that in 7635865

@jameslamb
Copy link
Collaborator Author

Ugh it looks like AGAIN someone has turned on this setting that requires a re-review after a push:

Screenshot 2025-08-10 at 9 42 06 PM

I cannot merge this and see the following:

Merging is blocked
Waiting on 1 reapproval from someone other than the last pusher. Review from StrikerRUS is stale because it was submitted before the most recent code changes.

Previous times we've asked for this to be turned off and it seemed to have been for a bit:

It looks like I don't have permission to change the relevant ruleset: https://github.com/microsoft/LightGBM/settings/rules/5351760

I suspect it has to be a Microsoft employee.

@letmaik can you please help us with this? Either turn this off or tell us "no, this is Microsoft policy, you just have to live with it"?

As I said in #6872 (comment), this will really slow us down in this project :/

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Reapproving.

@jameslamb
Copy link
Collaborator Author

Thanks very much @StrikerRUS !

I hope someone from Microsoft will be able to finally and permanently resolve this branch protection issue for us, having to re-approve like this is even more demand on the already limited time we're giving to this project as volunteers 😫

@jameslamb jameslamb merged commit 97ff708 into master Aug 13, 2025
49 checks passed
@jameslamb jameslamb deleted the ci/rstcheck-precommit branch August 13, 2025 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants