Port twisted.python.usage to Python 3

[Jonathan Ballet]

This is needed for running most of the Twisted scripts.

[Jonathan Ballet]

The porting is relatively trivial:

• I had to add a few tests for functions which I modified in twisted.python.usage
• I added util.padTo for Python 3 as well, it was explicitly removed for some reasons. I added tests which are running for both Python 2.7 and Python 3

[Jonathan Ballet]

Replace usage of twisted.python.text's wordWrap

[Jonathan Ballet]

I've just read about https://twistedmatrix.com/trac/ticket/6341#comment:7 and so I replace text.wordWrap by textwrap.wrap. I'll notify #6341.

[Adi Roiban]

Instead of

class PadToTest(unittest.TestCase): 

I think we should have... to comply with coding standards

class TestPadTo(unittest.TestCase): 

Same for FlagFunctionTest and OptionsInternalTest

---

Just a personal taste for tests

Instead of

self.assertEqual([None] * 3, res)

Have

self.assertEqual([None, None, None], res)

The reason is that test don't have test and that code should be easy to read to avoid any errors.

---

I don't see where padTo is used . Why do we need this method?

---

I don't think that we need to include L{util.padTo in docstring for padTo test since the TestCase docstring should already state that all tests are for this function.

---

Does the patch contains a newsfile :) ?

---

I don't like try/except fail in tests. Why not use self.assertRaises ?

with self.assertRaises(SomeException) as context:
   sut.doDomething()

self.assertEqual('bla', context.exception.message)

---

You can remove test/check/should from test docstring for FlagFunctionTest. Docstrings for padTo are much better :)

---

# Aliases points towards the longest method name 

I think we can safely remove this comment as the docstring is clear.

---

For this kind of docstrings.

test_tooManyArguments
   """Check a method which takes too many arguments."""

and

test_tooManyArgumentsAndSpecificErrorMessage(self): 
    """ 
    Check a method which takes too many arguments, with a specific error 
    message.

Check what? Please try to repharse the docstring as it is not clear what the test should check.

"Specific error message". Do we need the 'specific' part? By reading the tests, it should be clear what error is raised.

This is a test and we should already assume that it is written to check something.

The test look like a check for a missing / unknown options.

Maybe something like this

test_flagFunctionUnknownOption():
    """
    When requested option does not exists usage.UsageError is raised.
    """

---

class OptionsInternalTest(unittest.TestCase)

Do we need a special test case for testing internal stuff? Why not create a single test case to test both internal and external staff.

In an ideal work we should only write tests for external / public API.

As much as possible I prefer to have all tests for a class in a single testcase.

---

For test_optionsAliasesOrder , I think that the test is ok, but I am not sure if we should call a private method.

Maybe usage.Options() provides another public method which can be used for checking this behavior. If there is no such public method, then test is fine.

---

Thanks for the work on this.

Please see my comments.

I might be wrong in some places and I might forget something... but hope a new round will bring this patch closer to merge :)

Thanks!

PS: As long as ticket is in python3 milestone, I don't think it needs extra keywords.

[Jean-Paul Calderone]

Thanks for your review adiroiban!

Here is a meta-review to help inform your future reviews.

Instead of

class PadToTest(unittest.TestCase): 

I think we should have... to comply with coding standards

{{{
class TestPadTo(unittest.TestCase): 
}}}

Where did you find this in the coding standard? As far as I know there's no mention of this.

test_tooManyArguments
    """Check a method which takes too many arguments."""

The coding standard does forbid this formatting of docstrings, though. """ goes on its own line.

As much as possible I prefer to have all tests for a class in a single testcase.

Other people may have their own preferences regarding this. :) In any event, the coding standard doesn't mandate one way or the other. Many of the review comments here are personal preferences - other developers may have other personal preferences, remember that as a reviewer yours don't hold any extra weight. :) In particular, for code that's already been written consider the extra effort that the author must now go to to update their code to match you preferences. This may not be the best use of their time.

Thanks again.

[Adi Roiban]

Thanks exarkun for the meta-review! I feel I am missing a tutoring process for new reviewers.

Agreed and understood.

multani sorry for the personal preference noise. Feel free to ignore my comments.

I fully agree that my personal preferences don't hold extra wait!

I tried to be clear what is wrong and what are only personal preferences.

I was trying to share best practices and experience while in the same time doing a review. For the future, I will only focus on review work.

I do consider the effort done by writing that code, but I also consider the effort required to read and maintain that code in the future.

I really don't want to enforce my personal preferences and my only intention is to suggest improvements which would benefit all.

---

I was wrong about PadToTest and TestPadTo. I was a personal thing. I use TestSomething for writing tests and SomethingTestCase to write cases shared by multiple tests.

---

Regarding grouping tests, I prefer that way since this can reduce the risk of creating multiple test cases for the same code. It also makes filtering easier for quick TDD runs.

---

Thanks again and sorry for the trouble.

[Jean-Paul Calderone]

Thanks again and sorry for the trouble.

No trouble at all. Thank you again for the review. :)

[Jonathan Ballet]

Updated docstrings in twisted/test/test_usage.py

[Jonathan Ballet]

Thanks for having a look adiroiban!

Replying to adiroiban:

Instead of

class PadToTest(unittest.TestCase): 

I think we should have... to comply with coding standards

class TestPadTo(unittest.TestCase): 

Same for FlagFunctionTest and OptionsInternalTest

Additionally to what exarkun said, I actually would have written the same, but I just followed the convention used elsewhere in the module, and Test was prefixed, instead of being suffixed.

I don't see where padTo is used . Why do we need this method?

padTo is used here and here.

I don't think that we need to include L{util.padTo in docstring for padTo test since the TestCase docstring should already state that all tests are for this function.

That's mostly for documentation purpose: if someone ever browses the API doc for these tests functions, he can directly jump to the right function. In any case, I prefer to add markup if something can be markup-ed.

Does the patch contains a newsfile :) ?

Damned, I was pretty sure I added one :( Well, I just checked, and there's actually an empty topfile, but Trac doesn't show it...

I don't like try/except fail in tests. Why not use self.assertRaises ?

with self.assertRaises(SomeException) as context:
   sut.doDomething()

self.assertEqual('bla', context.exception.message)

I don't like it either, but Twisted redefines assertRaises and it is not a context-manager anymore :( That looks like a bug in the test class, actually. Cf. ​http://twistedmatrix.com/trac/browser/tags/releases/twisted-13.2.0/twisted/trial/_synctest.py#L313

You can remove test/check/should from test docstring for FlagFunctionTest. Docstrings for padTo are much better :)

---

# Aliases points towards the longest method name 

I think we can safely remove this comment as the docstring is clear.

You are right, I added it while I was still figuring out my tests. It's removed now.

For this kind of docstrings.

test_tooManyArguments
   """Check a method which takes too many arguments."""

and

test_tooManyArgumentsAndSpecificErrorMessage(self): 
    """ 
    Check a method which takes too many arguments, with a specific error 
    message.

Check what? Please try to repharse the docstring as it is not clear what the test should check.

I agree it's not very clear, but the function I test isn't also really in the first place:

• flagFunction takes a method as a parameter
• it starts with a verbe but doesn't do anything
• return a boolean as 0 or 1

I updated the docstrings, if you feel it's still not right, please suggest more appropriate.

"Specific error message". Do we need the 'specific' part? By reading the tests, it should be clear what error is raised.

I changed this docstring.

This is a test and we should already assume that it is written to check something.

Well, I guess I meant "This function checks...", since it both returns something and "checks" that the value is correct (and raises an exception if it's not.)

The test look like a check for a missing / unknown options.

Maybe something like this

test_flagFunctionUnknownOption():
    """
    When requested option does not exists usage.UsageError is raised.
    """

That's not exactly what flagFunction does actually. :) I've been trying to come up with a good docstring, but either I'm too tired for this right now, or this function is kind of weird to explain.

class OptionsInternalTest(unittest.TestCase)

Do we need a special test case for testing internal stuff? Why not create a single test case to test both internal and external staff.

As much as possible I prefer to have all tests for a class in a single testcase.

Well, that would mean refactoring all the test cases in this module actually. This patch is about porting the module from Python 2 to Python 3, things have been done differently in the past and changing this fact is not part of this particular ticket. I don't really like either, but c'est la vie.

In an ideal work we should only write tests for external / public API.

In ideal world, maybe. But I disagree in practice. :)

For test_optionsAliasesOrder , I think that the test is ok, but I am not sure if we should call a private method.

Maybe usage.Options() provides another public method which can be used for checking this behavior. If there is no such public method, then test is fine.

I just wanted to be sure that the code I ported was still functioning as before, especially since I changed slightly the semantics of the sorting function. If you look at where the method I'm testing is called, I don't think I can do otherwise :)

Thanks for the work on this.

Please see my comments.

I might be wrong in some places and I might forget something... but hope a new round will bring this patch closer to merge :)

I will push a new patch with some updated docstrings.

PS: As long as ticket is in python3 milestone, I don't think it needs extra keywords.

The triaging of these tickets is a bit messy, so yeah, I'm not sure where to put what and how :/

[Adi Roiban]

I was doing review only based on diff. My bad.

For the future I will do review based on full code.

There is a ticket for context manager #5339 and would try to fix it.

---

Regarding padTo usage. I see. All good. Many thanks for the explanation.

I originally saw your comment I added util.padTo for Python 3 as well, it was explicitly removed for some reasons. but could not make sense out of it. Not your fault really!

---

Docstring are much better now! Thanks! flagFunction name and behavior is strange but reading the docstring, I can not understand what is going on there.

---

In ideal world, maybe. But I disagree in practice. :)

I just wanted to be sure that the code I ported was still functioning as before, especially since I changed slightly the semantics of the sorting function. If you look at where the method I'm testing is called, I don't think I can do otherwise :)

Agreed and understood! I just wanted to make sure we try the ideal world first :)

Can we rely on Options.synonyms for this test? This is public member. I know that is updated from _gather_parameters + other 2 methods but should the external API care about this internal implementation?

---

Minor style comment. For L{usage.flagFunction} in test docstring. I was suggesting not to repeat it at all in docstring for tests, but only mention it once in testcase docstring (which is already done).

But I am fine with current version.

---

From my point of view this is good for merge.

Thanks!

[Jonathan Ballet]

Use public API for testing + update docstring

[Jonathan Ballet]

Thanks for the review adiroiban!

Replying to adiroiban:

Agreed and understood! I just wanted to make sure we try the ideal world first :)

Can we rely on Options.synonyms for this test? This is public member. I know that is updated from _gather_parameters + other 2 methods but should the external API care about this internal implementation?

I changed the test so that it uses the Options.synonyms instead. I'm not terribly excited by this change, since it means there's even more code than before which is run just to exercise the sorting behavior I changed for the port.

Minor style comment. For L{usage.flagFunction} in test docstring. I was suggesting not to repeat it at all in docstring for tests, but only mention it once in testcase docstring (which is already done).

I let it like this: it makes the test function clearer by clearly stating what is being tested. Removing the function name doesn't really improve anything, especially for this function which is tricky enough by itself.

[Adi Roiban]

OK. Many thanks for the changes. Approved.

[habnabit]

(In [42247]) Branching to twisted.python.usage-python3-7038.

[habnabit]

(In [42251]) Apply patch from multani. refs #7038

[habnabit]

​Forced a build.

[habnabit]

Oops, ​twistedchecker and everything else too is failing.

[Alex Gaynor]

This patch looks mostly correct to me, however I think there are likely to be a handful of conflicts (a bunch of the syntax errors have been fxied). Also, I believe it'd be better to fix __hash__ in the manner of #7165.

(Obviously the tests need to be fixed though, :-))

[Jonathan Ballet]

Replying to habnabit:

​twistedchecker

Seriously?...

I mean, I really have to add all these docstrings + @params stuff to this class only used for tests, for a kind of function which is already doubtful?

Well, I did it. That is really useful now, thank you twistedchecker.

[Jonathan Ballet]

Replying to Alex:

This patch looks mostly correct to me, however I think there are likely to be a handful of conflicts (a bunch of the syntax errors have been fxied). Also, I believe it'd be better to fix __hash__ in the manner of #7165.

(Obviously the tests need to be fixed though, :-))

I rebased the patch on top of trunk. I removed what I did for the __hash__ function for the fix you did in #7165 instead. I guess it would have been better to merge all these modifications in one go instead of creating #7165, but anyway.

I removed at the same time the failing test, which was testing this particular implementation of __hash__ (and was using assertLess from Python 2.7+, hence the test failures on Python 2.6)

Thanks for the review!

[Jonathan Ballet]

Rebased on top of trunk@42331, fixed test failures on 2.6, pleased twistedchecker

[Jonathan Ballet]

Replying to habnabit:

Oops, ​twistedchecker and everything else too is failing.

Thank you for reviewing this patch habnabit. Sorry for the harsh tone in 16, but that just seems like a lost of time. Anyway, I should have tested this patch on 2.6 in the first place.

I uploaded a new version of the patch, which should fix the problems reported. It's now based on top of trunk@42331.

Thanks again!

[Alex Gaynor]

test_tooManyArguments and test_tooManyArgumentsAndSpecificErrorMessage currently pass if no exception is raised.

Besides that, this patch looks GTG and should be landed.

[Jonathan Ballet]

Fix tests passing even if no exception raised

[Jonathan Ballet]

Replying to Alex:

test_tooManyArguments and test_tooManyArgumentsAndSpecificErrorMessage currently pass if no exception is raised.

Oh right. This is fixed now. Thanks for pointing this out!

[Alex Gaynor]

Besides the need to run on the buildbots, and a topfile, this looks ready to land.

Thank you!

[Jean-Paul Calderone]

(In [42378]) Branching to 'twisted.python.usage-python3-7038-2'

[Jean-Paul Calderone]

(In [42379]) apply 7038-t.p.usage-py3-6.patch

refs #7038

[Jean-Paul Calderone]

Thanks. I noticed a couple other minor things. I'll address and re-submit for review.

[Jean-Paul Calderone]

Running out of steam, hope that's a bit better.

[Alex Gaynor]

This looks good to me. Please land.

[Jean-Paul Calderone]

(In [42392]) Merge twisted.python.usage-python3-7038-2

Author: multani, exarkun Reviewer: adiroiban, Alex, exarkun Fixes: #7038

Port twisted.python.usage to Python 3.