Skip to content

bpo-34081: Fix wrong example link that was linking to distutils #8248

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

Merged
merged 5 commits into from
Oct 21, 2018

Conversation

tirkarthi
Copy link
Member

@tirkarthi tirkarthi commented Jul 11, 2018

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
Copy link
Member Author

tirkarthi commented Jul 11, 2018

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
Copy link
Member Author

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

Copy link
Contributor

@BoboTiG BoboTiG left a comment

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
Copy link
Member Author

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

Thanks

Copy link
Contributor

@BoboTiG BoboTiG left a comment

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
Copy link
Member Author

tirkarthi commented Oct 20, 2018

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
Copy link
Member

@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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

Copy link
Member

@JulienPalard JulienPalard left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants