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

Added doctest encoding command line option #2101

Merged
merged 8 commits into from Nov 30, 2016

Conversation

Projects
None yet
4 participants
@wheerd
Copy link
Contributor

wheerd commented Nov 29, 2016

I added a command line option to specify the encoding of doctest (.txt/.rst) files.
This should fix the error described in #540 which I am also having on my windows machine.

Manuel Krebber added some commits Nov 29, 2016

Manuel Krebber
Added some tests for --docstring-encoding option. Added option to spe…
…cify encoding for internal testdir._makefile() for the tests.
def test_encoding_latin1(self, testdir):
"""Test support for --doctest-encoding option.
"""
testdir._makefile(".txt", ["""

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Nov 29, 2016

Member

at first glance i wonder if this shouldn't use unicode marked strings like u"..."

i believe the actual encoding can be a parametrize parameter, and we might want to add an exotic one just to be on the paranoid side

This comment has been minimized.

Copy link
@wheerd

wheerd Nov 29, 2016

Author Contributor

That would break the code for Python 3.0 I guess because iirc it didn't have the unicode prefix anymore.

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Nov 29, 2016

Member

@wheerd python 3.3 added it back for more pleasant dual codebases

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 29, 2016

Coverage Status

Coverage increased (+0.0008%) to 92.713% when pulling d254c6b on wheerd:doctest-encoding into 36eb5b3 on pytest-dev:features.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 29, 2016

Coverage Status

Coverage increased (+0.0008%) to 92.713% when pulling 929912d on wheerd:doctest-encoding into 36eb5b3 on pytest-dev:features.

'üäö'
testdir._makefile(".txt", [u"""
>>> len(u'üäö')
3

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Nov 29, 2016

Member

i propose making those a format string adding u in front of it for py2, and leaving it out for py3
that way the output can be tested

This comment has been minimized.

Copy link
@wheerd

wheerd Nov 30, 2016

Author Contributor

In Python 2, they will be formatted as escape sequences: u'\x84\x94\x81'. Probably I can just repr(value) to the files.

@@ -43,6 +43,10 @@ def pytest_addoption(parser):
action="store_true", default=False,
help="ignore doctest ImportErrors",
dest="doctest_ignore_import_errors")
group.addoption("--doctest-encoding",

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Nov 29, 2016

Member

I think this would be better off being a ini option instead:

  1. If one has to use this option, then usually all doctest.txt files will have the same encoding (if the developers want to keep their sanity that is) so it makes sense to configure that in pytest.ini;
  2. If really desired, one can use the -o option to override ini options in the command-line.
  3. It is usually not a good idea to require users to pass certain command-line options to make the test suite pass, and in which case the users don't pass the correct option the test-suite will fail in mysterious ways;
@@ -129,6 +129,52 @@ def test_multiple_patterns(self, testdir):
'*1 passed*',
])

def test_encoding_ascii(self, testdir):

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Nov 29, 2016

Member

I think you could use @pytest.mark.parametrize on the tests to avoid repeating yourself. Something like (already incorporating my request to change to an ini option):

import pytest

@pytest.mark.parametrize('string, encoding', [
    (u'foo', 'ascii'),
    (u'üäö', 'latin1'),
    (u'üäö', 'utf-8'),
])
def test_encoding_latin1(testdir, string, encoding):
    """Test support for doctest_encoding option.
    """
    testdir.makeini("""
        [pytest]
        doctest_encoding={0}
    """.format(encoding))
    testdir._makefile(".txt", u"""
        >>> len(u'{0}')
        3
    """.format(string), encoding=encoding)

    result = testdir.runpytest()
    result.stdout.fnmatch_lines([
        '*1 passed*',
    ])

EDIT: fixed a comment left by accident

@nicoddemus
Copy link
Member

nicoddemus left a comment

Other than my comments, please add a CHANGELOG entry under New Features and add yourself to the CHANGELOG. 😁

Manuel Krebber added some commits Nov 30, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 30, 2016

Coverage Status

Coverage decreased (-0.03%) to 92.68% when pulling 1f62e5b on wheerd:doctest-encoding into 36eb5b3 on pytest-dev:features.

@@ -11,6 +11,13 @@ can change the pattern by issuing::
on the command line. Since version ``2.9``, ``--doctest-glob``
can be given multiple times in the command-line.

You can specify the encoding that will be used for those doctest files
using the ``--doctest-encoding`` command line option::

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Nov 30, 2016

Member

Please update the docs to mention the doctest_encoding ini option instead.

This comment has been minimized.

Copy link
@wheerd

wheerd Nov 30, 2016

Author Contributor

Sorry, forgot about that.

doctest_encoding={0}
""".format(encoding))
doctest = u"""
>>> u"{}"

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Nov 30, 2016

Member

The use of {} is not supported by py26 and is breaking the CI, please change that to {0}, {1}, etc.

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Nov 30, 2016

I think we're almost there, thanks again for the PR!

Manuel Krebber added some commits Nov 30, 2016

@@ -11,6 +11,17 @@ can change the pattern by issuing::
on the command line. Since version ``2.9``, ``--doctest-glob``
can be given multiple times in the command-line.

You can specify the encoding that will be used for those doctest files
using the ``doctest_encoding`` ini option:

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Nov 30, 2016

Member

One last thing, please add this:

You can specify the encoding that will be used for those doctest files
using the ``doctest_encoding`` ini option:

.. versionadded:: 3.1

.. code-block:: ini

To convey to the users that this is new and was added in 3.1.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 30, 2016

Coverage Status

Coverage decreased (-0.03%) to 92.68% when pulling f8fef07 on wheerd:doctest-encoding into 36eb5b3 on pytest-dev:features.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 30, 2016

Coverage Status

Coverage decreased (-0.03%) to 92.68% when pulling f8fef07 on wheerd:doctest-encoding into 36eb5b3 on pytest-dev:features.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Nov 30, 2016

lovely - this is shaping up, good work

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 30, 2016

Coverage Status

Coverage decreased (-0.03%) to 92.68% when pulling 2edfc80 on wheerd:doctest-encoding into 36eb5b3 on pytest-dev:features.

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Nov 30, 2016

The py27-expect failure in AppVeyor is unrelated so this is good to merge.

Thanks again @wheerd!

@nicoddemus nicoddemus merged commit 669332b into pytest-dev:features Nov 30, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

This was referenced Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.