Opened 4 years ago
Closed 4 years ago
#7038 enhancement closed fixed (fixed)
Port twisted.python.usage to Python 3
Reported by: | Jonathan Ballet | Owned by: | Jean-Paul Calderone |
---|---|---|---|
Priority: | normal | Milestone: | Python-3.x |
Component: | core | Keywords: | |
Cc: | Branch: |
branches/twisted.python.usage-python3-7038-2
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | exarkun, habnabit |
Description
This is needed for running most of the Twisted scripts.
Attachments (6)
Change History (34)
Changed 4 years ago by
Attachment: | 7038-t.p.usage-py3-1.patch added |
---|
comment:1 Changed 4 years ago by
Keywords: | review added |
---|
Changed 4 years ago by
Attachment: | 7038-t.p.usage-py3-2.patch added |
---|
Replace usage of twisted.python.text's wordWrap
comment:2 Changed 4 years ago by
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.
comment:3 follow-up: 7 Changed 4 years ago by
Keywords: | python3 review removed |
---|---|
Owner: | set to Jonathan Ballet |
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.
comment:4 Changed 4 years ago by
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.
comment:5 Changed 4 years ago by
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.
comment:6 Changed 4 years ago by
Thanks again and sorry for the trouble.
No trouble at all. Thank you again for the review. :)
Changed 4 years ago by
Attachment: | 7038-t.p.usage-py3-3.patch added |
---|
Updated docstrings in twisted/test/test_usage.py
comment:7 Changed 4 years ago by
Keywords: | review added |
---|
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?
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 nameI 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 :/
comment:8 follow-up: 9 Changed 4 years ago by
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!
Changed 4 years ago by
Attachment: | 7038-t.p.usage-py3-4.patch added |
---|
Use public API for testing + update docstring
comment:9 Changed 4 years ago by
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.
comment:11 Changed 4 years ago by
Author: | → habnabit |
---|---|
Branch: | → branches/twisted.python.usage-python3-7038 |
(In [42247]) Branching to twisted.python.usage-python3-7038.
comment:14 follow-ups: 16 18 Changed 4 years ago by
Oops, twistedchecker and everything else too is failing.
comment:15 follow-up: 17 Changed 4 years ago by
Keywords: | review removed |
---|
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, :-))
comment:16 Changed 4 years ago by
Replying to habnabit:
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.
comment:17 Changed 4 years ago by
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!
Changed 4 years ago by
Attachment: | 7038-t.p.usage-py3-5.patch added |
---|
Rebased on top of trunk@42331, fixed test failures on 2.6, pleased twistedchecker
comment:18 Changed 4 years ago by
Keywords: | review added |
---|
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!
comment:19 follow-up: 20 Changed 4 years ago by
Keywords: | review removed |
---|
test_tooManyArguments
and test_tooManyArgumentsAndSpecificErrorMessage
currently pass if no exception is raised.
Besides that, this patch looks GTG and should be landed.
Changed 4 years ago by
Attachment: | 7038-t.p.usage-py3-6.patch added |
---|
Fix tests passing even if no exception raised
comment:20 Changed 4 years ago by
Keywords: | review added |
---|
Replying to Alex:
test_tooManyArguments
andtest_tooManyArgumentsAndSpecificErrorMessage
currently pass if no exception is raised.
Oh right. This is fixed now. Thanks for pointing this out!
comment:21 Changed 4 years ago by
Besides the need to run on the buildbots, and a topfile, this looks ready to land.
Thank you!
comment:22 Changed 4 years ago by
Owner: | changed from Jonathan Ballet to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:23 Changed 4 years ago by
Author: | habnabit → exarkun, habnabit |
---|---|
Branch: | branches/twisted.python.usage-python3-7038 → branches/twisted.python.usage-python3-7038-2 |
(In [42378]) Branching to 'twisted.python.usage-python3-7038-2'
comment:25 Changed 4 years ago by
Keywords: | review removed |
---|---|
Status: | assigned → new |
Thanks. I noticed a couple other minor things. I'll address and re-submit for review.
comment:26 Changed 4 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Jean-Paul Calderone to Alex Gaynor |
Running out of steam, hope that's a bit better.
comment:27 Changed 4 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Alex Gaynor to Jean-Paul Calderone |
This looks good to me. Please land.
comment:28 Changed 4 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
The porting is relatively trivial:
twisted.python.usage
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