bpo-27200: fix several doctests by marco-buttu · Pull Request #604 · python/cpython

marco-buttu · 2017-03-10T19:48:50Z

This PR partially fixes bpo-27200. Partially and not completely, because I followed the suggestion of @ezio-melotti to split the patch in several patches. This PR is the 3rd of the series. The first one was #240 and the second one #401.

To run the doctests (from the Doc directory):

$ sphinx-build -b doctest . build/doctest \
library/urllib.parse.rst \
library/functions.rst \
library/ipaddress.rst \
library/reprlib.rst \
library/shlex.rst \
library/email.compat32-message.rst \
whatsnew/3.2.rst

mention-bot · 2017-03-10T19:48:53Z

@marco-buttu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ncoghlan, @eliben, @birkenfeld, @vsajip and @bitdancer to be potential reviewers.

bitdancer

I've only glanced at the non-email package stuff, and I haven't tested any of it myself, but this looks reasonable except for my one comment.

bitdancer · 2017-03-10T20:16:04Z

Doc/library/email.compat32-message.rst

-         ...     msg = message_from_binary_file(f)
-         >>> from email.iterators import _structure
+         from email import message_from_binary_file
+         with open('../Lib/test/test_email/data/msg_16.txt', 'rb') as f:


This is obviously fragile. Someone added the testsetup, and presumably it worked for them when they ran it and this change would break it for them. We should compute the path relative to email.__file__. That won't work in all cases, but it will work in more cases.

@bitdancer in the last commit I made the change you suggested me. Thank you very much for your time!

berkerpeksag · 2017-03-22T09:39:51Z

Doc/library/shlex.rst

   cases where you cannot use a list.

-   This idiom would be unsafe::
+   This idiom would be unsafe:


Wouldn't using "::" be more correct here? I understand the use of "::" is a bit inconsistent in the rest of the file, but I prefer to keep it as is if it doesn't break any doctests.

Hi @berkerpeksag , to make the test after The quoting is compatible with UNIX shells to pass, remote_command should be defined. In order for remote_command to be defined, we need to execute this test and the next one, that is why I changed ::`` to ``:. If you want to use :: instead of :, than you have to skip the test that uses the remote_command name, but I do not see any good reason for that.

See my comment below about skipping tests explicitly.

Mmm, I think there is a misunderstanding here. I do not want to skip this test group, but I want it to be executed. If we skip these tests then remote_command will not be defined and the next test groups (after The quoting is compatible with UNIX shells) will fail.

Hello @berkerpeksag, only this point is still open. Here I prefere to execute the test, because this example defines remote_command , that is used in a subsequent example. In this way all tests pass. If you want us to skip this test, we have to skip the subsequent tests that use theremote_command name. Let me know which option you prefere. Thank you very much for your time :-)

Sorry, I forgot to comment on this earlier. Your change looks fine to me. I still need to run the tests locally.

berkerpeksag · 2017-03-22T09:42:23Z

Doc/library/ipaddress.rst

-   ...                        IPv4Network)
+
+   import ipaddress
+   from ipaddress import (ip_network, IPv4Address, IPv4Interface,


Can we use a hanging indentation to make future diffs clearer?

from ipaddress import ( ip_network, IPv4Address, IPv4Interface, IPv4Network, )

Thanks @berkerpeksag, done :-)

marco-buttu · 2017-03-22T12:08:50Z

Doc/whatsnew/3.2.rst

 The :func:`~math.erf` function computes a probability integral or `Gaussian
 error function <https://en.wikipedia.org/wiki/Error_function>`_.  The
-complementary error function, :func:`~math.erfc`, is ``1 - erf(x)``:
+complementary error function, :func:`~math.erfc`, is ``1 - erf(x)``::


I skipped the test because it can fail (on my machine I got 0.6826894921370859 instead of 0.682689492137086).

This is not terribly important, but it would be nice to be more explicit and use something like

.. doctest:: :options: +SKIP

to skip a doctest.

Thanks @berkerpeksag :-) I have a doubt about consistency. I agree with you that in general using the doctest directive and the SKIP option is a better choice than using ::. But about consistency the PEP8 says that "Consistency within one module or function is the most important". In this file there is no one directive doctest. To skip an entire test group the :: are provided. To skip just one test inside a group, the # doctest: +SKIP comment is provided. That is why I preferred to not use the doctest directive in this case. But if you really want to use the doctest directive, I will use it. Just let me know. Thank you very much for your time!

Thanks for the lecture, but I'm well aware of what PEP 8 says.

@berkerpeksag I did perceive Marco as lecturing, but rather giving the context for why he had a doubt about your suggestion.

Marco: the difference here with what PEP8 is talking about is that we are now (starting from approximately last year's PyCon sprint) actually trying to execute doctests, where we've never been worried about that in the past (and the use of : and :: may have been somewhat random). So as far as skipping doctests or not, there is no true pre-existing style to follow. We ought instead to figure out what we want to do documentation wide, and do it. And I do like Berker's suggestion, since it makes it explicit. I myself can never remember which colon form does which.

@bitdancer thank you very much for the explanation :-) I hope at the end of this task we will write some guidelines for the doctest, in order to have only one way to write them. In this commit I deindented the code in order to allign it to the doctest directive. @berkerpeksag for sure I am not able to do any lecture. If you misunderstand my question I suppose it is becose of my poor English. Sorry for that :( Thank you very much for all the time you are spending in these reviews and answering my questions :-)

berkerpeksag · 2017-04-26T15:34:45Z

Thanks, Marco. I'll wait for @bitdancer's approval since he requested changes to the PR earlier.

bitdancer · 2017-04-26T19:46:42Z

Doc/library/email.compat32-message.rst

+         import email
+         from email import message_from_binary_file
+         from os.path import join, dirname
+         lib_dir = dirname(email.__file__).rstrip('/email')


rstrip is wrong here. It doesn't you do what you think it does. Intstead, apply dirname twice.

I made the change, thanks @bitdancer :-) It was also Unix-like biased... Two independent errors in just one line :-O Thank you very much for your time and patience. These reviews are priceless :-)

berkerpeksag · 2017-04-27T12:23:44Z

Thanks!

marco-buttu added 7 commits March 10, 2017 19:29

bpo-27200: fix email.compat32-message.rst doctests

45d0735

bpo-27200: fix functions.rst doctests

165a439

bpo-27200: fix ipaddress.rst doctests

ba6efbc

bpo-27200: fix reprlib.rst doctests

38e42a0

bpo-27200: fix whatsnew/3.2.rst doctests

446db45

bpo-27200: fix urllib.parse.rst doctests

f47f49e

bpo-27200: fix shlex.rst doctests

5ad6d1b

the-knights-who-say-ni added the CLA signed label Mar 10, 2017

bitdancer requested changes Mar 10, 2017

View reviewed changes

bpo-27200: get the absolute file path in testsetup

73aacd4

marco-buttu mentioned this pull request Mar 11, 2017

bpo-27200: fix pathlib, ssl, turtle and weakref doctests #616

Merged

berkerpeksag requested changes Mar 22, 2017

View reviewed changes

marco-buttu added 2 commits March 22, 2017 13:03

bpo-27200: use hanging indentation in ipaddress.rst

b94e308

bpo-27200: skipped a test that can fail.

5c38137

marco-buttu commented Mar 22, 2017

View reviewed changes

marco-buttu mentioned this pull request Mar 26, 2017

bpo-27200: fix unittest.mock doctests. #821

Closed

marco-buttu added 2 commits April 13, 2017 17:21

bpo-27200: use doctest SKIP option instead of ::

fbae43f

bpo-27200: fix wrong doctest directive syntax

2aab0c4

berkerpeksag approved these changes Apr 26, 2017

View reviewed changes

bitdancer requested changes Apr 26, 2017

View reviewed changes

bpo-27200: fix bug in testsetup

2cd4aec

bitdancer approved these changes Apr 27, 2017

View reviewed changes

berkerpeksag merged commit e65fcde into python:master Apr 27, 2017

Uh oh!

Comments

Conversation

marco-buttu commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Mar 10, 2017

Uh oh!

bitdancer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marco-buttu Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marco-buttu Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Apr 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Apr 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marco-buttu commented Mar 10, 2017 •

edited

Loading

marco-buttu Apr 24, 2017 •

edited

Loading

marco-buttu Apr 13, 2017 •

edited

Loading