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

GH-39990: [Docs][CI] Add sphinx-lint for docs linting #40022

Merged
merged 5 commits into from
May 1, 2024

Conversation

amoeba
Copy link
Member

@amoeba amoeba commented Feb 10, 2024

What changes are included in this PR?

This adds developer tooling to the repo for linting the docs by adding the sphinx-lint tool to archery and our pre-commit hooks. In both locations, only two rules are enabled at the moment (Discussed in #40006): trailing-whitespace and missing-final-newline.

This PR also fixes the individual issues covered by the new tooling.

Are these changes tested?

Yes, though manually. I tested this works by running archery lint --docs and pre-commit without and without changes that should get caught by the rules. It works as expected.

Are there any user-facing changes?

Yes,

  1. Developers that use pre-commit hooks will see a change in behavior when they modify docs
  2. Developers using archery will see a new --docs option in archery lint
  3. Developers working on the docs may see CI failures related to the new checks

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Looks good!

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@amoeba amoeba marked this pull request as ready for review March 16, 2024 20:27
.pre-commit-config.yaml Show resolved Hide resolved
dev/archery/archery/cli.py Show resolved Hide resolved
@kou
Copy link
Member

kou commented Mar 18, 2024

Could you rebase on main?

@amoeba
Copy link
Member Author

amoeba commented Mar 19, 2024

Thanks @kou. I need to fix a couple merge conflicts during rebase and I'll ping you once this is ready.

@amoeba
Copy link
Member Author

amoeba commented Mar 19, 2024

Hi @kou, I rebased and pushed a few extra changes. I just tested the behavior archery lint --docs and pre-commit and things are working as expected. This is ready for another look.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

docs/requirements.txt Show resolved Hide resolved
ci/conda_env_sphinx.txt Show resolved Hide resolved
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Mar 19, 2024
@kou
Copy link
Member

kou commented Mar 19, 2024

Could you update the description before we merge this?

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Mar 19, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review awaiting committer review Awaiting committer review and removed awaiting changes Awaiting changes awaiting committer review Awaiting committer review awaiting change review Awaiting change review labels Mar 20, 2024
@amoeba
Copy link
Member Author

amoeba commented Mar 20, 2024

Sorry for not doing that before asking for another review @kou. Description updated. Feedback from the last 24hours has all been addressed as well.

sphinx-lint --disable all --enable trailing-whitespace,missing-final-newline
- Adds sphinx-lint to archery as archer lint --docs
- Adds archery lint --docs to pre-commit hook
- Adds sphinx-lint to appropriate setup.py, conda lock, and requirements.txt
@amoeba
Copy link
Member Author

amoeba commented May 1, 2024

I rebased, pushed two fixes in recent changes and will merge this once CI passes.

@amoeba amoeba merged commit 0f7e9af into apache:main May 1, 2024
12 of 14 checks passed
@amoeba amoeba removed the awaiting committer review Awaiting committer review label May 1, 2024
@amoeba
Copy link
Member Author

amoeba commented May 1, 2024

Merged. There were two failing CI checks but both were unrelated (recent pyarrow asof join failures, both have their own issues).

Thanks for the review here @kou and @jorisvandenbossche.

@jorisvandenbossche
Copy link
Member

Thanks @amoeba! Are there other rules we want to consider enabling as follow-ups?

Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 0f7e9af.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 45 possible false positives for unstable benchmarks that are known to sometimes produce them.

tolleybot pushed a commit to tmct/arrow that referenced this pull request May 2, 2024
…0022)

### What changes are included in this PR?

This adds developer tooling to the repo for linting the docs by adding the sphinx-lint tool to archery and our pre-commit hooks. In both locations, only two rules are enabled at the moment (Discussed in apache#40006): `trailing-whitespace` and `missing-final-newline`.

This PR also fixes the individual issues covered by the new tooling. 

### Are these changes tested?

Yes, though manually. I tested this works by running `archery lint --docs` and `pre-commit` without and without changes that should get caught by the rules. It works as expected.

### Are there any user-facing changes?

Yes,

1. Developers that use pre-commit hooks will see a change in behavior when they modify docs
2. Developers using archery will see a new --docs option in `archery lint`
3. Developers working on the docs may see CI failures related to the new checks

* Closes: apache#39990
* GitHub Issue: apache#39990

Authored-by: Bryce Mecum <[email protected]>
Signed-off-by: Bryce Mecum <[email protected]>
tolleybot pushed a commit to tmct/arrow that referenced this pull request May 4, 2024
…0022)

### What changes are included in this PR?

This adds developer tooling to the repo for linting the docs by adding the sphinx-lint tool to archery and our pre-commit hooks. In both locations, only two rules are enabled at the moment (Discussed in apache#40006): `trailing-whitespace` and `missing-final-newline`.

This PR also fixes the individual issues covered by the new tooling. 

### Are these changes tested?

Yes, though manually. I tested this works by running `archery lint --docs` and `pre-commit` without and without changes that should get caught by the rules. It works as expected.

### Are there any user-facing changes?

Yes,

1. Developers that use pre-commit hooks will see a change in behavior when they modify docs
2. Developers using archery will see a new --docs option in `archery lint`
3. Developers working on the docs may see CI failures related to the new checks

* Closes: apache#39990
* GitHub Issue: apache#39990

Authored-by: Bryce Mecum <[email protected]>
Signed-off-by: Bryce Mecum <[email protected]>
rok pushed a commit to tmct/arrow that referenced this pull request May 8, 2024
…0022)

### What changes are included in this PR?

This adds developer tooling to the repo for linting the docs by adding the sphinx-lint tool to archery and our pre-commit hooks. In both locations, only two rules are enabled at the moment (Discussed in apache#40006): `trailing-whitespace` and `missing-final-newline`.

This PR also fixes the individual issues covered by the new tooling. 

### Are these changes tested?

Yes, though manually. I tested this works by running `archery lint --docs` and `pre-commit` without and without changes that should get caught by the rules. It works as expected.

### Are there any user-facing changes?

Yes,

1. Developers that use pre-commit hooks will see a change in behavior when they modify docs
2. Developers using archery will see a new --docs option in `archery lint`
3. Developers working on the docs may see CI failures related to the new checks

* Closes: apache#39990
* GitHub Issue: apache#39990

Authored-by: Bryce Mecum <[email protected]>
Signed-off-by: Bryce Mecum <[email protected]>
rok pushed a commit to tmct/arrow that referenced this pull request May 8, 2024
…0022)

### What changes are included in this PR?

This adds developer tooling to the repo for linting the docs by adding the sphinx-lint tool to archery and our pre-commit hooks. In both locations, only two rules are enabled at the moment (Discussed in apache#40006): `trailing-whitespace` and `missing-final-newline`.

This PR also fixes the individual issues covered by the new tooling. 

### Are these changes tested?

Yes, though manually. I tested this works by running `archery lint --docs` and `pre-commit` without and without changes that should get caught by the rules. It works as expected.

### Are there any user-facing changes?

Yes,

1. Developers that use pre-commit hooks will see a change in behavior when they modify docs
2. Developers using archery will see a new --docs option in `archery lint`
3. Developers working on the docs may see CI failures related to the new checks

* Closes: apache#39990
* GitHub Issue: apache#39990

Authored-by: Bryce Mecum <[email protected]>
Signed-off-by: Bryce Mecum <[email protected]>
@amoeba
Copy link
Member Author

amoeba commented May 9, 2024

Good reminder, thanks @jorisvandenbossche. I think quite a few would be good to add. I've filed #41611 for that work.

vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
…0022)

### What changes are included in this PR?

This adds developer tooling to the repo for linting the docs by adding the sphinx-lint tool to archery and our pre-commit hooks. In both locations, only two rules are enabled at the moment (Discussed in apache#40006): `trailing-whitespace` and `missing-final-newline`.

This PR also fixes the individual issues covered by the new tooling. 

### Are these changes tested?

Yes, though manually. I tested this works by running `archery lint --docs` and `pre-commit` without and without changes that should get caught by the rules. It works as expected.

### Are there any user-facing changes?

Yes,

1. Developers that use pre-commit hooks will see a change in behavior when they modify docs
2. Developers using archery will see a new --docs option in `archery lint`
3. Developers working on the docs may see CI failures related to the new checks

* Closes: apache#39990
* GitHub Issue: apache#39990

Authored-by: Bryce Mecum <[email protected]>
Signed-off-by: Bryce Mecum <[email protected]>
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.

[Docs][CI] Investigate linting C++ and other docs in CI
3 participants