Skip to content

Docs: make Sphinx stricter #7603

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 2, 2021
Merged

Docs: make Sphinx stricter #7603

merged 5 commits into from
Sep 2, 2021

Conversation

andreasabel
Copy link
Member

Added flags to sphinx-build to make it stricter:

  • -n: more warnings
  • -W: warnings to errors
  • -E: rebuild

This will hopefully prevent errors creeping in to the docs (e.g. broken references such as #7469).

Thanks to these flags, I fixed two issues (see additional commits).

@andreasabel
Re haskell#7469: doc: unified references to PATH environment variable
f931e4f 
Found four variants, picked the first:

  1. ``PATH``
  2. `PATH`
  3. ``$PATH``
  4. :envvar:`PATH`

The last (4) produced an error with sphinx:
```
doc/cabal-project.rst:644: WARNING: 'envvar' reference target not found: PATH
```
@andreasabel
Re haskell#7469: doc: typo found by deciphering sphinx warning
ac9baa1 
Wasn't immediate to get to the error from the warning
```
doc/cabal-project.rst:2: WARNING: Duplicate ID: "cfg-field-type".
```
(wrong line number, "cfg-field-type" is not greppable).
@andreasabel
Re haskell#7469: sphinx-build with flag -n (nitpick)
2720a89
@andreasabel
Re haskell#7469: sphinx-build with flag -W -E --keep-going
d690d9b 
Strict regime: turn warnings to errors (-W), rebuild from scratch (-E).
This was referenced Aug 31, 2021
Closed
Merged
Mikolaj
Mikolaj approved these changes Aug 31, 2021
View reviewed changes
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Not nearly enough experience on my part, but LGTM. Citing from IRC:

somebody asked (here? on gibhub) if we build docs as part of CI; we don't, right? should we? I guess we should, unless it takes forever?

@Mikolaj
Copy link
Member

Mikolaj commented Aug 31, 2021

OK, got the question and the answer, so we now need a PR: #7586 (comment)

@Mikolaj
Copy link
Member

Mikolaj commented Aug 31, 2021

@Mergifyio backport 3.6

@mergify
Copy link
Contributor

mergify bot commented Aug 31, 2021

Command backport 3.6: pending

Waiting for the pull request to get merged

@fgaz
Copy link
Member

fgaz commented Aug 31, 2021

I think we shouldn't have -W in the makefile, only in CI (like it's done for -Werror for haskell code).

So in the makefile:

SPHINX_FLAGS += -n --keep-going -E

And in CI:

SPHINX_FLAGS=-W make users-guide

@andreasabel
Copy link
Member Author

@fgaz wrote:

I think we shouldn't have -W in the makefile, only in CI (like it's done for -Werror for haskell code).

Is the Makefile shipped? I thought it is developers only.

So in the makefile:

SPHINX_FLAGS += -n --keep-going -E

Can I then still override the flags with make SPHINX_FLAGS= users-guide?

@fgaz
Copy link
Member

fgaz commented Aug 31, 2021 •
edited
Loading

Is the Makefile shipped? I thought it is developers only.

that's right, but I still think it'd be nicer to not fail on warnings when writing docs so that one can still preview them while fixing the warnings (like for haskell). this would also avoid breakage in case new warnings are added to sphinx (though it looks like we're pinning it)

Can I then still override the flags with make SPHINX_FLAGS= users-guide?

yes. from what I understand make users-guide SPHINX_FLAGS=... overrides, while SPHINX_FLAGS=... make users-guide makes the += append

fgaz
fgaz approved these changes Sep 1, 2021
View reviewed changes
Copy link
Member

@fgaz fgaz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

By the way that above is just a weak preference so I'll just give an approve. It can also be done later when someone adds CI for the guide

@andreasabel
Copy link
Member Author

@fgaz

but I still think it'd be nicer to not fail on warnings when writing docs so that one can still preview them while fixing the warnings

If it turns out that -W is a nuisance to developers, it can be changed back.
Note also that with --keep-going you will still see all the errors (at least so it is promised). An advantage of -W by default (as opposed to just in the CI) is a quicker feedback cycle (if it passes locally, it also passed the CI).

@Mikolaj Mikolaj merged commit 067bb43 into haskell:master Sep 2, 2021
@Mikolaj
Copy link
Member

Mikolaj commented Sep 2, 2021

Merged over the corpse of AppVeyor CI.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 2, 2021

Thank you!

@mergify mergify bot mentioned this pull request Sep 2, 2021
Merged
@mergify
Copy link
Contributor

mergify bot commented Sep 2, 2021

Command backport 3.6: success

Backports have been created

@andreasabel andreasabel deleted the sphinx-nitpick branch September 2, 2021 10:21
Mikolaj added a commit that referenced this pull request Sep 2, 2021
@Mikolaj
Merge pull request #7612 from haskell/mergify/bp/3.6/pr-7603
fc53282 
Docs: make Sphinx stricter (backport #7603)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mikolaj Mikolaj Mikolaj approved these changes

@fgaz fgaz fgaz approved these changes

@emilypi emilypi emilypi approved these changes

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andreasabel @Mikolaj @fgaz @emilypi