Skip to content
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

Doctests #12

Merged
merged 2 commits into from
May 12, 2017
Merged

Doctests #12

merged 2 commits into from
May 12, 2017

Conversation

videlec
Copy link
Collaborator

@videlec videlec commented Apr 28, 2017

The branch

  • fix the travis script
  • converts all Sage doctest to Python doctests
  • fix many doctest failures with Python 2.7

See also trac #22899 for the doctests that will be directly moved to Sage.

@videlec videlec mentioned this pull request Apr 28, 2017
4 tasks
@defeo defeo merged commit 132b393 into master May 12, 2017
@jdemeyer
Copy link
Collaborator

Why did you change the formatting of the doctests (apart from just changing sage: to >>>)? This is going to look bad if we would process the documentation with Sphinx.

@videlec
Copy link
Collaborator Author

videlec commented May 15, 2017

The sage: to >>> is the minimum to have doctest works. The EXAMPLES and TESTS were Sage specific. What is the minimum to have sphinx compatible syntax? (some websites mention :Example: and others .. doctests::)

@jdemeyer
Copy link
Collaborator

What is the minimum to have sphinx compatible syntax?

The double colon as in EXAMPLES:: (but also Some other text:: would work), followed by an indented block.

@videlec
Copy link
Collaborator Author

videlec commented May 16, 2017

What is the minimum to have sphinx compatible syntax?

The double colon as in EXAMPLES:: (but also Some other text:: would work), followed by an indented block.

Easy to fix. I propose to postpone this change after #13 (Python 3 compatibility).

@jdemeyer could you modify the travis script to check that building the documentation with sphinx works?

@jdemeyer
Copy link
Collaborator

could you modify the travis script to check that building the documentation with sphinx works?

I'll try to do that this week but I cannot promise anything.

@jdemeyer
Copy link
Collaborator

Actually, it turns out that Sphinx does understand the docstring style introduced in this PR. So nothing needs to be fixed...

@videlec
Copy link
Collaborator Author

videlec commented May 18, 2017

very good. Could you have a look at #13 ?

@jdemeyer
Copy link
Collaborator

could you modify the travis script to check that building the documentation with sphinx works?

Done! And it actually works.

@jdemeyer jdemeyer deleted the doctests branch July 28, 2017 08:52
[]
Tests:

>>> from six.moves import range
Copy link
Member

Choose a reason for hiding this comment

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

I just realised this doctests adds a useless dependency on six. Without the import, the doctest works just fine in Py2&3.
However, I'm not sure about the original intent of the test. Maybe replacing range(10) with list(range(10)) is closer to it.
Opinions? I shall start a separate pull request for this.

@jdemeyer
Copy link
Collaborator

I just realised this doctests adds a useless dependency on six.

You could do a similar thing as some other Python 2/3 doctests and just use sys.version (or whatever it's called). The idea is to get xrange in Python 2 and range in Python 3.

@defeo
Copy link
Member

defeo commented Jul 31, 2017

Oh, I see. I had interpreted it the other way round.

So a one line replacement compatible with both 2 & 3 would be objtogen(i for i in range(10)). But then this is redundant with the following doctest, we could as well remove it. I have the impression that it would be more interesting to replace it with objtogen(list(range(10)).

@jdemeyer
Copy link
Collaborator

jdemeyer commented Aug 1, 2017

So a one line replacement compatible with both 2 & 3 would be objtogen(i for i in range(10)). But

Not quite since (i for i in range(10)) is not the same as neither xrange(10) or range(10). Both are iterables yielding the same numbers, but it's not the same thing.

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.

None yet

3 participants