bpo-19382: Adding test cases for module tabnanny. #851

Merged
merged 1 commit into from Jun 14, 2018

Conversation

Projects
None yet
9 participants
@ultimatecoder
Contributor

ultimatecoder commented Mar 27, 2017

  • Description: Unit tests for standard module tabnanny. Tests are added for almost all the functionality except tabnanny.Whitespace class.

  • Reason leaving Whitespace:? I found the module contains mathematical calculations. I will try to understand it first and then write tests for it. I think it will be good to do that in another branch.

  • Testing strategy: Whitebox

  • BPOs

https://bugs.python.org/issue19382

@DimitrisJim

This comment has been minimized.

Show comment
Hide comment
@DimitrisJim

DimitrisJim Mar 27, 2017

Contributor

Hi @ultimatecoder, you'll also need to remove tabnanny from the untested tuple in test_sundry. This is one of the failures AppVeyor (and soon Travis, I'd guess), reports.

Contributor

DimitrisJim commented Mar 27, 2017

Hi @ultimatecoder, you'll also need to remove tabnanny from the untested tuple in test_sundry. This is one of the failures AppVeyor (and soon Travis, I'd guess), reports.

Lib/test/test_tabnanny.py
+
+ Use this method to assert expected values of `stdout` and `stderr` after
+ running tabsize.check() on given `dir` or `file` path. Because
+ tabsize.check() captures arised exceptions and writes to `stdout` and

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

Just write “captures exceptions” (or change arised → raised).

What is “tabsize”? Did you mean “tabnanny.check”?

@vadmium

vadmium Mar 27, 2017

Member

Just write “captures exceptions” (or change arised → raised).

What is “tabsize”? Did you mean “tabnanny.check”?

Lib/test/test_tabnanny.py
+ Returns path of the created file. Returned
+ file path can be feeded to
+ `tabnanny.check()`. Here, the expression
+ `*` at function name is dipicting the

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

feeded → fed
dipicting → depicting

@vadmium

vadmium Mar 27, 2017

Member

feeded → fed
dipicting → depicting

Lib/test/test_tabnanny.py
+ * dir_or_file : A path to file or directory. This argument will be
+ feeded directly to tabnanny.check().
+ * out : Expacted `stdout` after running tabsize.check().
+ * err : Expacted `stderr` after running tabsize.check().

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

feeded → fed
Expacted → Expected

@vadmium

vadmium Mar 27, 2017

Member

feeded → fed
Expacted → Expected

Lib/test/test_tabnanny.py
+ """Directory containing few error free python source code files.
+
+ Because order of files returned by `os.lsdir()` is not fixed, verifying
+ existance of each output lines at `stdout` using `in` operator.

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

verifying → verify the
existance → existence

@vadmium

vadmium Mar 27, 2017

Member

verifying → verify the
existance → existence

Lib/test/test_tabnanny.py
+ self._verify_tabnanny_check(file_path)
+
+ def test_correct_directory_verbose(self):
+ """Directory containing few error free python source code files.

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

few → a few [otherwise, the implication is that most of the files have errors: few are error-free]

@vadmium

vadmium Mar 27, 2017

Member

few → a few [otherwise, the implication is that most of the files have errors: few are error-free]

Lib/test/test_tabnanny.py
+ """Testing `tabnanny.format_witnesses()`."""
+
+ def test_format_whitenesses(self):
+ """Asserting formatter result by givinig various input samples."""

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

giving

@vadmium

vadmium Mar 27, 2017

Member

giving

Lib/test/test_tabnanny.py
+ self._validate_cmd(stderr=stderr)
+
+ def test_quiet_flag(self):
+ """Should not display less when quite mode is on."""

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

quiet mode?
“less” looks wrong, unless you meant “Should display less”

@vadmium

vadmium Mar 27, 2017

Member

quiet mode?
“less” looks wrong, unless you meant “Should display less”

Lib/test/test_tabnanny.py
+ def test_quiet_flag(self):
+ """Should not display less when quite mode is on."""
+ file_path = create_python_file(SOURCE_CODES["nannynag_errored"])
+ stdout = b"%s\n" % (file_path.encode('ascii'),)

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

Don’t you get CRLF on Windows?

@vadmium

vadmium Mar 27, 2017

Member

Don’t you get CRLF on Windows?

Lib/test/test_tabnanny.py
+ def test_with_errored_file(self):
+ """Should displays error when errored python file is given."""
+ file_path = create_python_file(SOURCE_CODES["wrong_indented"])
+ stderr = b"%r: Indentation Error: " % (file_path,)

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

In Python 3, b"%r" is equivalent to calling the “ascii” function. I expect the test will fail if the file path has non-ASCII characters. It may be better to only test that an error is being reported, but tolerate variations in the message.

@vadmium

vadmium Mar 27, 2017

Member

In Python 3, b"%r" is equivalent to calling the “ascii” function. I expect the test will fail if the file path has non-ASCII characters. It may be better to only test that an error is being reported, but tolerate variations in the message.

Lib/test/test_tabnanny.py
+ def test_command_usage(self):
+ """Should display usage on no arguments."""
+ path = findfile('tabnanny.py')
+ stderr = b'Usage: %s [-v] file_or_directory ...' % (path.encode('ascii'))

This comment has been minimized.

@vadmium

vadmium Mar 27, 2017

Member

What if the path is non-ASCII? Again, just check the general form of the output and don’t be too specific.

@vadmium

vadmium Mar 27, 2017

Member

What if the path is non-ASCII? Again, just check the general form of the output and don’t be too specific.

This comment has been minimized.

@ultimatecoder

ultimatecoder Mar 28, 2017

Contributor

I don't think this https://github.com/python/cpython/blob/master/Lib/tempfile.py#L144 generates anything Non-ASCII.

@ultimatecoder

ultimatecoder Mar 28, 2017

Contributor

I don't think this https://github.com/python/cpython/blob/master/Lib/tempfile.py#L144 generates anything Non-ASCII.

This comment has been minimized.

@vadmium

vadmium Mar 31, 2017

Member

No, but the OS can provide a parent directory for temporary files that has non-ASCII path components. It seems relatively common on Windows that the temporary directory is within a user’s profile, and the profile directory can be a localized non-ASCII name. Top Google result: https://bugzilla.mozilla.org/show_bug.cgi?id=260034.

@vadmium

vadmium Mar 31, 2017

Member

No, but the OS can provide a parent directory for temporary files that has non-ASCII path components. It seems relatively common on Windows that the temporary directory is within a user’s profile, and the profile directory can be a localized non-ASCII name. Top Google result: https://bugzilla.mozilla.org/show_bug.cgi?id=260034.

This comment has been minimized.

@ultimatecoder

ultimatecoder Apr 6, 2017

Contributor

Many thanks for affording time for reviewing this. I will agree with you on this point. I have updated code. Will you please review again?

@ultimatecoder

ultimatecoder Apr 6, 2017

Contributor

Many thanks for affording time for reviewing this. I will agree with you on this point. I have updated code. Will you please review again?

@vadmium

This comment has been minimized.

Show comment
Hide comment
@vadmium

vadmium Mar 27, 2017

Member

Check out the existing patches and reviews you linked. You may be repeating some of the same problems, e.g. CRLF on Windows.

Member

vadmium commented Mar 27, 2017

Check out the existing patches and reviews you linked. You may be repeating some of the same problems, e.g. CRLF on Windows.

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Mar 28, 2017

Contributor

@vadmium Many thanks for your comments. I am not native English speaker. I have fixed the mistakes. I have added comments to some comments. Please review my fixes. Thanks!

@DimitrisJim Thanks for your comment. I have removed tabnanny from untested modules list. Thanks!

Contributor

ultimatecoder commented Mar 28, 2017

@vadmium Many thanks for your comments. I am not native English speaker. I have fixed the mistakes. I have added comments to some comments. Please review my fixes. Thanks!

@DimitrisJim Thanks for your comment. I have removed tabnanny from untested modules list. Thanks!

@vadmium

This comment has been minimized.

Show comment
Hide comment
@vadmium

vadmium Apr 9, 2017

Member

Sorry but I don’t have an easy way to see your fixes relative to the old version I reviewed. I’m not really set up to properly review and push stuff with the new Git Hub setup, so I will leave this to someone else, or for another time in the future.

Member

vadmium commented Apr 9, 2017

Sorry but I don’t have an easy way to see your fixes relative to the old version I reviewed. I’m not really set up to properly review and push stuff with the new Git Hub setup, so I will leave this to someone else, or for another time in the future.

@@ -0,0 +1,348 @@
+"""Testing `tabnanny` module.
+

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

remove one empty line

@vstinner

vstinner May 4, 2017

Member

remove one empty line

This comment has been minimized.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Removed this. This is Github glitch.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Removed this. This is Github glitch.

Lib/test/test_tabnanny.py
+ * errored : Whitespace related problems present in file.
+"""
+from unittest import TestCase, mock
+from unittest.mock import patch

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Hum, I prefer to write mock.patch, please remove this import

@vstinner

vstinner May 4, 2017

Member

Hum, I prefer to write mock.patch, please remove this import

Lib/test/test_tabnanny.py
+from unittest.mock import patch
+import tabnanny
+import tokenize
+from tempfile import NamedTemporaryFile, TemporaryDirectory

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Please import tempfile and write tempfile.NamedTemporaryFile.

@vstinner

vstinner May 4, 2017

Member

Please import tempfile and write tempfile.NamedTemporaryFile.

Lib/test/test_tabnanny.py
+
+ It creates file with `.py` extension.
+
+ Returns: Absolute path of created code file."""

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

IMHO this docstring is useless. Usually, we don't comment much tests, they are not intented to end users.

@vstinner

vstinner May 4, 2017

Member

IMHO this docstring is useless. Usually, we don't comment much tests, they are not intented to end users.

Lib/test/test_tabnanny.py
+
+ Returns: Absolute path of created code file."""
+ with NamedTemporaryFile(mode='w', dir=dir,
+ delete=False, suffix=".py") as f:

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

If you only care of creating a temporary filename, use tempfile.mktemp() and then open() manually the file to create it. You can use open(tmpname, "x") to prevent race conditions.

@vstinner

vstinner May 4, 2017

Member

If you only care of creating a temporary filename, use tempfile.mktemp() and then open() manually the file to create it. You can use open(tmpname, "x") to prevent race conditions.

Lib/test/test_tabnanny.py
+ def test_errprint(self):
+ """Asserting result of `tabnanny.errprint()` by giving sample inputs."""
+ tests = [
+ {'args': ['first', 'second'], 'expected': 'first second\n'},

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Instead of a dict, you can simply use a tuple (args, expected)

@vstinner

vstinner May 4, 2017

Member

Instead of a dict, you can simply use a tuple (args, expected)

This comment has been minimized.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Don't think we should kill the readability by converting it to tuple. How about collections.namedtuple?

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Don't think we should kill the readability by converting it to tuple. How about collections.namedtuple?

This comment has been minimized.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Or How about this way?

        tests = [
         #  (Function arguments , Expected output)
            (['first', 'second'], 'first second\n'),
            (['first']          , 'first\n'),
            ([1, 2, 3]          , '1 2 3\n'),
            ([]                 , '\n')
        ]
@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Or How about this way?

        tests = [
         #  (Function arguments , Expected output)
            (['first', 'second'], 'first second\n'),
            (['first']          , 'first\n'),
            ([1, 2, 3]          , '1 2 3\n'),
            ([]                 , '\n')
        ]

This comment has been minimized.

@merwok

merwok Sep 1, 2017

Contributor

Keep tests simple:

for args, expected in [
    ([1], '1'),
  ]:
@merwok

merwok Sep 1, 2017

Contributor

Keep tests simple:

for args, expected in [
    ([1], '1'),
  ]:

This comment has been minimized.

@brettcannon

brettcannon Feb 22, 2018

Member

Yes, keep them simple. And don't use namedtuples except for keeping backwards-compatibility with an interface that used to take an indexed object and now takes an object with attributes.

@brettcannon

brettcannon Feb 22, 2018

Member

Yes, keep them simple. And don't use namedtuples except for keeping backwards-compatibility with an interface that used to take an indexed object and now takes an object with attributes.

Lib/test/test_tabnanny.py
+
+
+class TestNannyNag(TestCase):
+ """Testing `tabnanny.NannyNag` exception."""

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Not sure that this docstring is super useful.

@vstinner

vstinner May 4, 2017

Member

Not sure that this docstring is super useful.

Lib/test/test_tabnanny.py
+ self.assertEqual(stderr.getvalue() , test['expected'])
+
+
+class TestNannyNag(TestCase):

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

I don't think that it's worth it to declare one test case to test a single class/function. Try to put most or all tests into a single test case called "TabnannyTestCase".

TestCheck deserves its own test case since it has a setUp() method.

@vstinner

vstinner May 4, 2017

Member

I don't think that it's worth it to declare one test case to test a single class/function. Try to put most or all tests into a single test case called "TabnannyTestCase".

TestCheck deserves its own test case since it has a setUp() method.

Lib/test/test_tabnanny.py
+class TestNannyNag(TestCase):
+ """Testing `tabnanny.NannyNag` exception."""
+ def setUp(self):
+ self.tests = [

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Please move this list into the method directly.

@vstinner

vstinner May 4, 2017

Member

Please move this list into the method directly.

Lib/test/test_tabnanny.py
+ """Testing tabnanny.check()."""
+
+ def setUp(self):
+ tabnanny.verbose = 0 # Forcefully deactivating verbose mode.

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Unit tests must have zero side effect: you must save the old verbose value and restore it. Example:

self.addCleanup(setattr, tabnanny, 'verbose', tabnanny.verbose)
tabnanny.verbose = 0
@vstinner

vstinner May 4, 2017

Member

Unit tests must have zero side effect: you must save the old verbose value and restore it. Example:

self.addCleanup(setattr, tabnanny, 'verbose', tabnanny.verbose)
tabnanny.verbose = 0

This comment has been minimized.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Aha! a geek way 👍

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Aha! a geek way 👍

Lib/test/test_tabnanny.py
+ def setUp(self):
+ tabnanny.verbose = 0 # Forcefully deactivating verbose mode.
+
+ def _verify_tabnanny_check(self, dir_or_file, out="", err=""):

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

No need to make this method private, remove "_" prefix.

@vstinner

vstinner May 4, 2017

Member

No need to make this method private, remove "_" prefix.

Lib/test/test_tabnanny.py
+ * dir_or_file : A path to file or directory. This argument will be
+ fed directly to tabnanny.check().
+ * out : Expected `stdout` after running tabnanny.check().
+ * err : Expected `stderr` after running tabnanny.check().

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Again, I'm not sure that a long comment is needed for a simple unit test.

@vstinner

vstinner May 4, 2017

Member

Again, I'm not sure that a long comment is needed for a simple unit test.

This comment has been minimized.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

I think it is explaining why the method is here. Suggest a way for improving the docstring.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

I think it is explaining why the method is here. Suggest a way for improving the docstring.

This comment has been minimized.

@brettcannon

brettcannon Feb 22, 2018

Member

At least ditch the whole Arguments section; we don't use that style in the stdlib.

@brettcannon

brettcannon Feb 22, 2018

Member

At least ditch the whole Arguments section; we don't use that style in the stdlib.

Lib/test/test_tabnanny.py
+
+ def test_correct_file(self):
+ """A python source code file without any errors."""
+ file_path = create_tmp_python_file(SOURCE_CODES["error_free"])

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Who removes the temporary file? I suggest to put create_tmp_python_file() in the test case (or in a base class if it's needed by multiple test cases) and use self.addCleanup(support.unlink, tmpfile) to make sure that the temporary file is removed.

@vstinner

vstinner May 4, 2017

Member

Who removes the temporary file? I suggest to put create_tmp_python_file() in the test case (or in a base class if it's needed by multiple test cases) and use self.addCleanup(support.unlink, tmpfile) to make sure that the temporary file is removed.

This comment has been minimized.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Adding it to a method will be a poor idea. If I will add it to base class then we will have a base class with one method. Will you accept that?

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

Adding it to a method will be a poor idea. If I will add it to base class then we will have a base class with one method. Will you accept that?

This comment has been minimized.

@brettcannon

brettcannon Feb 22, 2018

Member

Yes, if the method needs to access information that will be on the instance then a class with a single method is fine. But if you aren't reusing it then just inline the function.

@brettcannon

brettcannon Feb 22, 2018

Member

Yes, if the method needs to access information that will be on the instance then a class with a single method is fine. But if you aren't reusing it then just inline the function.

@vstinner

I like the overall change, but I have a long list of requests :-)

Lib/test/test_tabnanny.py
+
+ @patch('tabnanny.NannyNag')
+ def test_with_correct_code(self, MockNannyNag):
+ """A python source code without any whitespace related problems."""

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

IMHO the docstring is useless.

@vstinner

vstinner May 4, 2017

Member

IMHO the docstring is useless.

Lib/test/test_tabnanny.py
+ exception = tabnanny.NannyNag
+ with open(file_path) as f:
+ tokens = tokenize.generate_tokens(f.readline)
+ self.assertRaises(exception, tabnanny.process_tokens, tokens)

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Move this assert out of the with block.

@vstinner

vstinner May 4, 2017

Member

Move this assert out of the with block.

This comment has been minimized.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

We can't move this out.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

We can't move this out.

This comment has been minimized.

@brettcannon

brettcannon Feb 22, 2018

Member

Why not? Is tokenize.generate_tokens() lazy (which isn't documented)?

@brettcannon

brettcannon Feb 22, 2018

Member

Why not? Is tokenize.generate_tokens() lazy (which isn't documented)?

This comment has been minimized.

@ultimatecoder

ultimatecoder Mar 18, 2018

Contributor

@brettcannon The tokenize.generate_tokens() is a lazy function (generator). Can you help to identify a way which allows putting assertRaises outside the with? Thanks!

@ultimatecoder

ultimatecoder Mar 18, 2018

Contributor

@brettcannon The tokenize.generate_tokens() is a lazy function (generator). Can you help to identify a way which allows putting assertRaises outside the with? Thanks!

Lib/test/test_tabnanny.py
+ """
+ rc, out, err = script_helper.assert_python_ok('-m', 'tabnanny', *args)
+ # Note: The `splitlines()` will solve the problem of CRLF(\r) added
+ # by OS Windows.

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

Use universal_newlines=True to normalize newlines. I also prefer to use text rather than bytes for stdout/stderr.

@vstinner

vstinner May 4, 2017

Member

Use universal_newlines=True to normalize newlines. I also prefer to use text rather than bytes for stdout/stderr.

This comment has been minimized.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

I don't think these chain of methods, assert_python_ok -> _assert_python -> run_python_until_end is allowing to pass universal_newlines keyword argument!

Reference: https://github.com/python/cpython/blob/master/Lib/test/support/script_helper.py#L94

Are you suggesting to replace script_helper.assert_python_ok() with subprocess.Popen() here?

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

I don't think these chain of methods, assert_python_ok -> _assert_python -> run_python_until_end is allowing to pass universal_newlines keyword argument!

Reference: https://github.com/python/cpython/blob/master/Lib/test/support/script_helper.py#L94

Are you suggesting to replace script_helper.assert_python_ok() with subprocess.Popen() here?

Lib/test/test_tabnanny.py
+ # Note: The `splitlines()` will solve the problem of CRLF(\r) added
+ # by OS Windows.
+ self.assertListEqual(out.splitlines(), stdout.splitlines())
+ self.assertEqual(rc, return_code)

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

assert_python_ok() already tests that returncode==0.

@vstinner

vstinner May 4, 2017

Member

assert_python_ok() already tests that returncode==0.

Lib/test/test_tabnanny.py
+class TestCommandLine(TestCase):
+ """Tests command line interface of `tabnanny`."""
+
+ def _validate_cmd(self, *args, return_code=0, stdout=b"", stderr=b""):

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

make this method public

@vstinner

vstinner May 4, 2017

Member

make this method public

Lib/test/test_tabnanny.py
+ def test_double_verbose_mode(self):
+ """Should display detailed error information if double verbose is on."""
+ file_path = create_tmp_python_file(SOURCE_CODES["nannynag_errored"])
+ stdout = b"checking %r ...\n" % (file_path,)

This comment has been minimized.

@vstinner

vstinner May 4, 2017

Member

For better readability, you can use textwrap.dedent() and a multiline """...""" indented string. See test_faulthandler for examples.

@vstinner

vstinner May 4, 2017

Member

For better readability, you can use textwrap.dedent() and a multiline """...""" indented string. See test_faulthandler for examples.

This comment has been minimized.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

This is a nice suggestion 👍 Are you expecting these multiline strings to convert into a regular expression? I think this way just looks fine.

@ultimatecoder

ultimatecoder May 5, 2017

Contributor

This is a nice suggestion 👍 Are you expecting these multiline strings to convert into a regular expression? I think this way just looks fine.

Lib/test/test_tabnanny.py
+class TestFormatWitnesses(TestCase):
+ """Testing `tabnanny.format_witnesses()`."""
+
+ def test_format_whitenesses(self):

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 5, 2017

Contributor

witnesses, not whitenesses

@JelleZijlstra

JelleZijlstra May 5, 2017

Contributor

witnesses, not whitenesses

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Feb 21, 2018

Member

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

Member

brettcannon commented Feb 21, 2018

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Feb 21, 2018

Contributor

@brettcannon Many thanks for your comment. I had tried to comment back at confusing points. I think @vstinner lost his interest in my solution. What efforts should I do to make it mergeable? Have a great day!

Contributor

ultimatecoder commented Feb 21, 2018

@brettcannon Many thanks for your comment. I had tried to comment back at confusing points. I think @vstinner lost his interest in my solution. What efforts should I do to make it mergeable? Have a great day!

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Feb 21, 2018

Member

@ultimatecoder if you think you have addressed all of the comments left by @vstinner then just leave a comment that says I have made the requested changes; please review again.

Member

brettcannon commented Feb 21, 2018

@ultimatecoder if you think you have addressed all of the comments left by @vstinner then just leave a comment that says I have made the requested changes; please review again.

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Feb 22, 2018

Contributor

@brettcannon I have done the requested changes. There were few which I would like to discuss with @vstinner according to his availability. He has pointed out few changes on which I am confused about. I have tried to write back as a part of reply comment in response to improvement comment. I hope I made a clear statement about the situation. Thanks!

Contributor

ultimatecoder commented Feb 22, 2018

@brettcannon I have done the requested changes. There were few which I would like to discuss with @vstinner according to his availability. He has pointed out few changes on which I am confused about. I have tried to write back as a part of reply comment in response to improvement comment. I hope I made a clear statement about the situation. Thanks!

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Mar 1, 2018

Contributor

@brettcannon Many thanks for your comments. I am under little work pressure. Please expect answers/improvements at the end of this week. Thanks!

Contributor

ultimatecoder commented Mar 1, 2018

@brettcannon Many thanks for your comments. I am under little work pressure. Please expect answers/improvements at the end of this week. Thanks!

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Mar 1, 2018

Member

@ultimatecoder no rush. We all have work that takes up our free time. 😄

Member

brettcannon commented Mar 1, 2018

@ultimatecoder no rush. We all have work that takes up our free time. 😄

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Mar 18, 2018

Contributor

@brettcannon I have tried updating the code according to expected changes. I am requesting you to help for finding a better way to assert the exception part coded here. Please write back with further changies expected by you. Many thaks!

Contributor

ultimatecoder commented Mar 18, 2018

@brettcannon I have tried updating the code according to expected changes. I am requesting you to help for finding a better way to assert the exception part coded here. Please write back with further changies expected by you. Many thaks!

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Mar 19, 2018

Member

@ultimatecoder You can use self.assertRaises() as a context manager:

with self.assertRaises(tabnanny.NannyNag):
    tabnanny.process_tokens(tokens)
Member

brettcannon commented Mar 19, 2018

@ultimatecoder You can use self.assertRaises() as a context manager:

with self.assertRaises(tabnanny.NannyNag):
    tabnanny.process_tokens(tokens)
@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Mar 23, 2018

Contributor

Hello @brettcannon ,
I have updated the code according to mentioned comments. I request to re-review. Have a great day!

Contributor

ultimatecoder commented Mar 23, 2018

Hello @brettcannon ,
I have updated the code according to mentioned comments. I request to re-review. Have a great day!

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Mar 23, 2018

Member

@ultimatecoder do note that there's a special phrase to say to trigger the bot to notify folks for a re-review. For now I manually updated the issue's labels to reflect your request for another review.

Member

brettcannon commented Mar 23, 2018

@ultimatecoder do note that there's a special phrase to say to trigger the bot to notify folks for a re-review. For now I manually updated the issue's labels to reflect your request for another review.

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Apr 9, 2018

Contributor

I have made the requested changes; please review again

Contributor

ultimatecoder commented Apr 9, 2018

I have made the requested changes; please review again

@bedevere-bot

This comment has been minimized.

Show comment
Hide comment
@bedevere-bot

bedevere-bot Apr 9, 2018

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon Apr 20, 2018

Member

@ultimatecoder I'm rather busy between now at PyCon US, but I promise that if no one else gets to it I will give you a review during the sprints at PyCon US.

Member

brettcannon commented Apr 20, 2018

@ultimatecoder I'm rather busy between now at PyCon US, but I promise that if no one else gets to it I will give you a review during the sprints at PyCon US.

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Apr 21, 2018

Contributor

@brettcannon Sure, thanks for reply 😀

Contributor

ultimatecoder commented Apr 21, 2018

@brettcannon Sure, thanks for reply 😀

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder May 15, 2018

Contributor

@brettcannon Reminding you for this PR. All the best for sprints 👍

Contributor

ultimatecoder commented May 15, 2018

@brettcannon Reminding you for this PR. All the best for sprints 👍

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon May 15, 2018

Member

@ultimatecoder no need as this is the top of the list for when I actually get time to start reviewing PRs 😉

Member

brettcannon commented May 15, 2018

@ultimatecoder no need as this is the top of the list for when I actually get time to start reviewing PRs 😉

@brettcannon

Nothing fundamentally wrong, just some things to help make the tests easier to manage in future years.

Lib/test/test_tabnanny.py
+ ([] , '\n')
+ ]
+
+ for args, expected in tests:

This comment has been minimized.

@vstinner

vstinner Jun 14, 2018

Member

Done

Lib/test/test_tabnanny.py
+ `verbose` mode of `tabnanny.verbose` asserts `stdout`.
+ """
+ with tempfile.TemporaryDirectory() as tmp_dir:
+ lines = ["%r: listing directory\n" % (tmp_dir,),]

This comment has been minimized.

@brettcannon

brettcannon May 15, 2018

Member

Use f-strings everywhere. 😄

@brettcannon

brettcannon May 15, 2018

Member

Use f-strings everywhere. 😄

This comment has been minimized.

@vstinner

vstinner Jun 14, 2018

Member

Done

Lib/test/test_tabnanny.py
+ """
+ with tempfile.TemporaryDirectory() as tmp_dir:
+ lines = ["%r: listing directory\n" % (tmp_dir,),]
+ with TemporaryPyFile(

This comment has been minimized.

@brettcannon

brettcannon May 15, 2018

Member

To make this easier to read, I would create the context managers outside the with statement:

file1 = TemporaryPyFile(...)
file2 = TemporaryPyFile(...)
with file1 as file1_path, file2 as file2_path:
    ...
@brettcannon

brettcannon May 15, 2018

Member

To make this easier to read, I would create the context managers outside the with statement:

file1 = TemporaryPyFile(...)
file2 = TemporaryPyFile(...)
with file1 as file1_path, file2 as file2_path:
    ...

This comment has been minimized.

@vstinner

vstinner Jun 14, 2018

Member

For this specific case, IMHO "file1 = ..." is better.

@vstinner

vstinner Jun 14, 2018

Member

For this specific case, IMHO "file1 = ..." is better.

Lib/test/test_tabnanny.py
+ def test_correct_directory(self):
+ """Directory which contains few error free python source code files."""
+ with tempfile.TemporaryDirectory() as tmp_dir:
+ with TemporaryPyFile(

This comment has been minimized.

@brettcannon

brettcannon May 15, 2018

Member

Move the TemporaryPyFile instantiation to its own line so you don't have to spread over multiple lines. (Same of the other lines where you were forced to spread a with statement over multiple lines.)

@brettcannon

brettcannon May 15, 2018

Member

Move the TemporaryPyFile instantiation to its own line so you don't have to spread over multiple lines. (Same of the other lines where you were forced to spread a with statement over multiple lines.)

This comment has been minimized.

@vstinner

vstinner Jun 14, 2018

Member

I looked at the new code and it's fine.

@vstinner

vstinner Jun 14, 2018

Member

I looked at the new code and it's fine.

Lib/test/test_tabnanny.py
+
+ def test_when_tokenize_tokenerror(self):
+ """A python source code file eligible for raising 'tokenize.TokenError'.
+ """

This comment has been minimized.

@brettcannon

brettcannon May 15, 2018

Member

Pull this up to the next line up (it's okay for it to spill over 80 columns).

@brettcannon

brettcannon May 15, 2018

Member

Pull this up to the next line up (it's okay for it to spill over 80 columns).

Lib/test/test_tabnanny.py
+ SOURCE_CODES["nannynag_errored"]
+ ) as file_path:
+ out = "%r: *** Line 3: trouble in tab city! ***\n" % (file_path,)
+ out += "offending line: '\\tprint(\"world\")\\n'\n"

This comment has been minimized.

@brettcannon

brettcannon May 15, 2018

Member

Don't worry too much about the exact output being formatted as it is. For instance, if someone tweaked the grammar of the sentence the test should still pass. Do still check for semantically important info, though, like that the offending line is in the output.

@brettcannon

brettcannon May 15, 2018

Member

Don't worry too much about the exact output being formatted as it is. For instance, if someone tweaked the grammar of the sentence the test should still pass. Do still check for semantically important info, though, like that the offending line is in the output.

This comment has been minimized.

@vstinner

vstinner Jun 14, 2018

Member

Maybe the test is too strict, but IMHO we have been too pendantic on this nice PR which has been proposed 3 months ago. tabnanny has currently no test. If the test fails tomorrow, we will just fix the test. No need to write the perfect test here.

@vstinner

vstinner Jun 14, 2018

Member

Maybe the test is too strict, but IMHO we have been too pendantic on this nice PR which has been proposed 3 months ago. tabnanny has currently no test. If the test fails tomorrow, we will just fix the test. No need to write the perfect test here.

Lib/test/test_tabnanny.py
+ def test_when_no_file(self):
+ """A python file which does not exist actually in system."""
+ file_path = 'no_file.py'
+ err = "%r: I/O Error: [Errno 2] No such file or directory: %r\n" % (

This comment has been minimized.

@brettcannon

brettcannon May 15, 2018

Member

You don't want to tie yourself to exception messages as they do change between versions. Just care about the right exception being raised.

@brettcannon

brettcannon May 15, 2018

Member

You don't want to tie yourself to exception messages as they do change between versions. Just care about the right exception being raised.

This comment has been minimized.

@ultimatecoder

ultimatecoder Jun 10, 2018

Contributor

I agree, but the code does not raise an exception reference: https://github.com/python/cpython/blob/master/Lib/tabnanny.py#L99
What way will be better to assert the working of mentioned function?

@ultimatecoder

ultimatecoder Jun 10, 2018

Contributor

I agree, but the code does not raise an exception reference: https://github.com/python/cpython/blob/master/Lib/tabnanny.py#L99
What way will be better to assert the working of mentioned function?

+ # `check_equal and type not in JUNK` condition at
+ # `tabnanny.process_tokens()`.
+
+ for key in ["tab_space_errored_1", "tab_space_errored_2"]:

This comment has been minimized.

@brettcannon

brettcannon May 15, 2018

Member

Sub tests would be great here.

@brettcannon

brettcannon May 15, 2018

Member

Sub tests would be great here.

This comment has been minimized.

@vstinner

vstinner Jun 11, 2018

Member

Done

@bedevere-bot

This comment has been minimized.

Show comment
Hide comment
@bedevere-bot

bedevere-bot May 15, 2018

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder May 16, 2018

Contributor

@brettcannon I am a bit busy with present work. I am sure, I will not be able to complete during this sprints. But I will be able to improve according to your comments in few days. Thanks for reviewing :)

Contributor

ultimatecoder commented May 16, 2018

@brettcannon I am a bit busy with present work. I am sure, I will not be able to complete during this sprints. But I will be able to improve according to your comments in few days. Thanks for reviewing :)

@brettcannon

This comment has been minimized.

Show comment
Hide comment
@brettcannon

brettcannon May 16, 2018

Member

@ultimatecoder no rush! I'm not expecting to take quite so long to do future reviews as I only need to review relative future changes.

Member

brettcannon commented May 16, 2018

@ultimatecoder no rush! I'm not expecting to take quite so long to do future reviews as I only need to review relative future changes.

@vstinner

Please remove Lib/test/.test_tabnanny.py.swo from your PR.

@vstinner

Oh, I am sorry, I forgot about your old PR :-( Here is a new review. Your PR is very close to be ready to be merged. Sorry again about the slow review :-(

Lib/test/test_tabnanny.py
+ """Asserting formatter result by giving various input samples."""
+ tests = [
+ ('Test' , 'at tab sizes T, e, s, t'),
+ ('' , 'at tab size '),

This comment has been minimized.

@vstinner

vstinner Jun 10, 2018

Member

Sorry but this style is not compliant with PEP 8, please strip spaces before ",".

@vstinner

vstinner Jun 10, 2018

Member

Sorry but this style is not compliant with PEP 8, please strip spaces before ",".

Lib/test/test_tabnanny.py
+
+ def __exit__(self, exc_type, exc_value, exc_traceback):
+ if exc_type is None:
+ unlink(self.file_path)

This comment has been minimized.

@vstinner

vstinner Jun 10, 2018

Member

You should always remove the created file.

@vstinner

vstinner Jun 10, 2018

Member

You should always remove the created file.

Lib/test/test_tabnanny.py
+
+ def validate_cmd(self, *args, stdout="", stderr=""):
+ """Common function to assert the behaviour of command line interface."""
+ _, out, err = script_helper.assert_python_ok('-m', 'tabnanny', *args)

This comment has been minimized.

@vstinner

vstinner Jun 10, 2018

Member

You may use:

proc = script_helper.spawn_python('-m', 'tabnanny', *args, text=True)
out, err = proc.communicate()
self.assertEqual(out, stdout)
self.assertEqual(err, stderr)
self.assertEqual(proc.returncode, 0)

text=True normalizes newlines to \n (Unix EOL).

@vstinner

vstinner Jun 10, 2018

Member

You may use:

proc = script_helper.spawn_python('-m', 'tabnanny', *args, text=True)
out, err = proc.communicate()
self.assertEqual(out, stdout)
self.assertEqual(err, stderr)
self.assertEqual(proc.returncode, 0)

text=True normalizes newlines to \n (Unix EOL).

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Jun 10, 2018

Contributor

@vstinner Thanks for re-observing this. I am working on the improvements suggested by you and @brettcannon 😃

Contributor

ultimatecoder commented Jun 10, 2018

@vstinner Thanks for re-observing this. I am working on the improvements suggested by you and @brettcannon 😃

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Jun 10, 2018

Contributor

@brettcannon I have updated the changes according to your comments. There is one comment in which you mentioned to assert the exception and not the output. I have added a reply to that comment with a reference. I request to provide your input on that. Thanks.

Contributor

ultimatecoder commented Jun 10, 2018

@brettcannon I have updated the changes according to your comments. There is one comment in which you mentioned to assert the exception and not the output. I have added a reply to that comment with a reference. I request to provide your input on that. Thanks.

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Jun 10, 2018

Contributor

@vstinner I have improved the code according to your suggestion. I request to re-review.

Contributor

ultimatecoder commented Jun 10, 2018

@vstinner I have improved the code according to your suggestion. I request to re-review.

Lib/test/test_tabnanny.py
+
+ def __exit__(self, exc_type, exc_value, exc_traceback):
+ unlink(self.file_path)
+ return exc_type is None

This comment has been minimized.

@vstinner

vstinner Jun 11, 2018

Member

I'm not sure that returning True is correct here. I just suggest to remove the "return" line.

@vstinner

vstinner Jun 11, 2018

Member

I'm not sure that returning True is correct here. I just suggest to remove the "return" line.

This comment has been minimized.

@ultimatecoder

ultimatecoder Jun 14, 2018

Contributor

I agree, returning True isn't making sense here. Thanks for commenting.

@ultimatecoder

ultimatecoder Jun 14, 2018

Contributor

I agree, returning True isn't making sense here. Thanks for commenting.

@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Jun 14, 2018

Contributor

@vstinner @brettcannon I have updated the code according to your comments. I request to proceed for further review. Thanks.

Contributor

ultimatecoder commented Jun 14, 2018

@vstinner @brettcannon I have updated the code according to your comments. I request to proceed for further review. Thanks.

@vstinner vstinner added the skip news label Jun 14, 2018

I checked all latest Brett's comments: all fixed

@vstinner vstinner merged commit dfa9643 into python:master Jun 14, 2018

9 checks passed

VSTS: Linux-PR Linux-PR_20180614.06 succeeded
Details
VSTS: Linux-PR-Coverage Linux-PR-Coverage_20180614.06 succeeded
Details
VSTS: Windows-PR Windows-PR_20180614.06 succeeded
Details
VSTS: docs docs_20180614.06 succeeded
Details
VSTS: macOS-PR macOS-PR_20180614.06 succeeded
Details
bedevere/issue-number Issue number 19382 found
Details
bedevere/news "skip news" label found
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ultimatecoder

This comment has been minimized.

Show comment
Hide comment
@ultimatecoder

ultimatecoder Jun 14, 2018

Contributor

@vstinner Thanks for taking time and reviewing this PR.

@vstinner Thank you for reviewing your PR and inspiring for continuing.

I have learned many this at the time of creating, maintaining this Patch.

Contributor

ultimatecoder commented Jun 14, 2018

@vstinner Thanks for taking time and reviewing this PR.

@vstinner Thank you for reviewing your PR and inspiring for continuing.

I have learned many this at the time of creating, maintaining this Patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment