◐ Shell
reader mode source ↗
Skip to content

Doc: -W flag for sphinx-build can be disabled#10303

Merged
JulienPalard merged 5 commits into
python:masterfrom
JulienPalard:doc-warn-optionally
Nov 3, 2018
Merged

Doc: -W flag for sphinx-build can be disabled#10303
JulienPalard merged 5 commits into
python:masterfrom
JulienPalard:doc-warn-optionally

Conversation

@JulienPalard

@JulienPalard JulienPalard commented Nov 2, 2018

Copy link
Copy Markdown
Member

Since 859c068, travis-ci is using -W via SPHINXOPTS so
we do not need it unconditionally in ALLSPHINXOPTS.

So I'm adding -W to Azure CI and droping it from ALLSPHINXOPTS.

Making -W optional would help with i18n builds (some versions, like
dev, are not actively maintained by translators, and can easily error, some countries are also less strict on sphinx errors and yields tons of thems).

cc @matrixise, @tirkarthi

@JulienPalard
Doc: Make -W (warn as error) more optional, still enforced by CI. …
fe1db29 
Since 859c068, CI is enforcing -W so
we do not need it unconditionally in Doc/Makefile.

Making -W optional would help with i18n builds (some versions, like
dev, are not actively maintained by translators, and can easily error).
@tirkarthi

tirkarthi commented Nov 2, 2018

Copy link
Copy Markdown
Member

The reason over the change was to catch warnings as errors locally. If it's caught during CI I am okay. It's just that it causes a round trip where make docs passes locally and fails in CI but helps for translated builds.

I have added a NEWS entry in my previous PR which can be deleted as part of this PR?

https://devguide.python.org/committing/#what-s-new-and-news-entries

If a change is reverted prior to release, then the corresponding entry is simply removed. Otherwise, a new entry must be added noting that the change has been reverted (e.g. when a feature is released in an alpha and then cut prior to the first beta).

Ref : https://github.com/python/cpython/pull/8248/files#diff-07576315d4856b4c89dcc90f4a2704e2

tirkarthi
tirkarthi approved these changes Nov 2, 2018
View reviewed changes

@tirkarthi tirkarthi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Approving the changes. News entry can be deleted to avoid confusion. Thanks :)

https://github.com/python/cpython/pull/8248/files#diff-07576315d4856b4c89dcc90f4a2704e2

@JulienPalard JulienPalard changed the title Doc: Make -W (warn as error) more optional, still enforced by CI. Nov 3, 2018
@JulienPalard

JulienPalard commented Nov 3, 2018

Copy link
Copy Markdown
Member Author

Implemented nice idea from @matrixise, @tirkarthi what do you think?

@tirkarthi

tirkarthi commented Nov 3, 2018

Copy link
Copy Markdown
Member

@JulienPalard I am little confused here since my Makefile knowledge is low but isn't it the same as using -W directly and we now just use SPHINXERRORHANDLING that refers to -W? Users who want to disable this set SPHINXERRORHANDLING environment variable to ''?

@JulienPalard

JulienPalard commented Nov 3, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Users who want to disable this set SPHINXERRORHANDLING environment variable to ''?

Almost, the precedence in Makefile is "command line beats makefile variables beats environment", so we'll have to pass it as command line, like:

make html SPHINXERRORHANDLING=''

Which is nice as we could also pass:

make html SPHINXERRORHANDLING='-W --keep-going'

in the CI to get more than a single error in the report.

@tirkarthi

tirkarthi commented Nov 3, 2018

Copy link
Copy Markdown
Member

@JulienPalard Thanks for the explanation. LGTM 👍

@matrixise

matrixise commented Nov 3, 2018

Copy link
Copy Markdown
Member

LGTM for me! You can merge it.

@JulienPalard JulienPalard merged commit f98c162 into python:master Nov 3, 2018
@JulienPalard JulienPalard deleted the doc-warn-optionally branch June 16, 2019 14:03
@ezio-melotti ezio-melotti mentioned this pull request Jun 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matrixise matrixise

@tirkarthi tirkarthi tirkarthi approved these changes

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JulienPalard @tirkarthi @matrixise @the-knights-who-say-ni @bedevere-bot