bpo-34081: Fix wrong example link that was linking to distutils by tirkarthi · Pull Request #8248 · python/cpython

Conversation

@tirkarthi

This PR fixes example link in Type Objects where examples was pointing to distutils examples . Distutils also had the same label as example. This was pointed out in the docs CI with the warning but it doesn't cause any error and hence was ignored. I think it will be better to turn on warning as errors in sphinx docs configuration so that these errors are caught in the CI run.

This is present only in master introduced as part of 9e7c921 and doesn't have to be backported.

Thanks

https://bugs.python.org/issue34081

@tirkarthi tirkarthi changed the title bpo34081: Fix wrong example link that was linking to distutils bpo-34081: Fix wrong example link that was linking to distutils

Jul 11, 2018

@tirkarthi

This can be turned on with -W in ALLSPHINXOPTS which gives the following error. Let me know if this needs to be added to the PR since current build is successful without warnings

(venv) ➜  Doc git:(master) ✗ make html
mkdir -p build
Building NEWS from Misc/NEWS.d with blurb
PATH=./venv/bin:$PATH sphinx-build -b html -d build/doctrees -W -D latex_elements.papersize=  . build/html
Running Sphinx v1.7.5
loading pickled environment... done
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 1 source files that are out of date
updating environment: 0 added, 2 changed, 0 removed
reading sources... [100%] whatsnew/changelog

Warning, treated as error:
/home/cpython/Doc/c-api/typeobj.rst:2327:duplicate label examples, other instance in /home/cpython/Doc/distutils/examples.rst
Makefile:43: recipe for target 'build' failed
make: *** [build] Error 2

@tirkarthi

I have added -W to have warnings as errors since it makes more sense in catching these type of errors. Let me know if it needs to be reverted.

Thanks

BoboTiG

Choose a reason for hiding this comment

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

Perhaps addind a NEWs entry would be a good thing, at least to let others know warnings are now activated.

@tirkarthi

Sorry that I missed this PR comment somehow. Added a NEWS entry.

Thanks

BoboTiG

Choose a reason for hiding this comment

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

Thank you for the patch :)

@tirkarthi

I think this is outdated now since the link was fixed in bpo-34962 along with doctest in CI but I think it's good to enable this in the makefile for docs so that errors are caught in the future while building docs locally though CI catches them too. @matrixise Thoughts?

@JulienPalard

@tirkarthi Hi, this is a good idea, we're already using -W while compiling the french translation.

Would you just remove the now-fixed-fix about examples and keep only the -W? I'll gladly merge this.

@tirkarthi

@JulienPalard Thanks for the feedback. I have reverted my change. Azure build fails though others pass. I have merged the latest master on my local machine and there doesn't seem to be any error on building locally. Attaching the log for reference.

17.txt

@tirkarthi

I think my Azure build uses Sphinx v1.6.7 which doesn't contain the directive though Travis uses Sphinx 1.8.1 and passes there. I will try to merge the latest master to see if it helps.

@tirkarthi

Ah, .azure-pipelines was not updated with the pinned version so there are already warnings in other PRs but since my PR makes warnings as error it fails. I think I get the error now and pin version in Azure to see if it helps :)

@tirkarthi

@JulienPalard Builds are green now :) You can review the changes.

JulienPalard

Choose a reason for hiding this comment

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

LGTM