This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author flox
Recipients amaury.forgeotdarc, barry, benjamin.peterson, brian.curtin, eric.araujo, esam, exarkun, ezio.melotti, flox, pitrou, rhettinger
Date 2010-01-29.19:15:38
SpamBayes Score 1.0158541e-14
Marked as misclassified No
Message-id <1264792540.44.0.973678542855.issue7092@psf.upfronthosting.co.za>
In-reply-to
Content
Thank you for the review and your comments.
Here, the replies.


>
> Amaury review - msg98491
>
> Here is my review of issue7092_syntax_imports_v3.diff:
>
> - test_itertools.py: please replace
>       [tuple([arg[i] if i < len(arg) else None for arg in args])
>        for i in range(max(map(len, args)))]
>   by something more readable (nested for loops for example)

It is the 3.x syntax.
--> No change

> - test_mailbox.py: why doesn't test_support.import_module('rfc822')
>   specify deprecated=True? maybe the module can be imported normally...

Added the "deprecated=True"

> - test_pyclbr.py:   replace self.assertTrue(key in obj) with assertIn()
> - test_wsgiref.py:  replace self.assertTrue(key in h.environ)
> - test_queue.py: test could be identical to the one in py3k.

Thanks
--> Done

> 
> Ezio review - msg98494
> 
> Here's mine about issue7092_check_warnings_v3.diff:
> 
> 1) test_callable should keep testing callable() and the warnings should be caught;

Done

> 2) in test_bsddb3 the problems should be correct in the module if possible and worth it (the module is deprecated);

Ok. I prepare a separate patch to fix bsddb3

> 3) next to the several '# Silence py3k warnings' it would be nice to have a note about what warning you are exactly silencing;
> 4) def test_deprecated_builtin_map -> test_deprecated_builtin_map_with_None, otherwise it seems that map is deprecated;

Done

> 5) in test[_deep]_copy I'm not entirely sure that the tests are equivalent using in (and if they are you should use assertIn);

Will use assertSetEqual, no need to reinvent the wheel

> 6) in test_socket I would keep callable, also shouldn't the raise in the next line raise a warning as well?;

Ok to keep callable. For the next line, I do not see the warning.

> 7)  the self.assertEqual(`u2`, `d2`) in test_userdict could just use repr() instead;

Done

> 8) a few tests in test_weakref should use assert[Not]In instead of assertTrue(x [not] in y).

Done

> 
> Ezio review - msg98507
> 
> A couple of comments about issue7092_syntax_imports_v3.diff too:
> 1) in test_copy you remove (k,v), but left the name 'k' even if now it represent the item and not the key;

Changed 'k'-> 'pair' (like 3.x)

> 2) in test_fractions you should probably use self.fail() instead of an assert;

The surrounding methods use "assert "
--> No change

> 3) on test_ftp you can use two separate lines and remove the ';';

Done

> 4) in test_pyclbr there's one extra 'f' in the comment in assertHaskey;

It is not an extra f. 'iff' = "if and only if" (not obvious for non-English people)

> 5) in test_xml_etree_c you can either leave callable() and catch the warning or use isinstance(x, collections.Callable) instead (there are also a few more places where callable was used too).

It is done this way in 3.x.
I propose to fix it separately (if a fix is required).



Now I prepare the updated patches.
I follow the proposal of Amaury: create a specific "test_support.silence_py3k_warnings()" context manager.
It replaces the "check_warnings" and "filterwarnings" of the proposed patches.
History
Date User Action Args
2010-01-29 19:15:40floxsetrecipients: + flox, barry, rhettinger, exarkun, amaury.forgeotdarc, pitrou, benjamin.peterson, ezio.melotti, eric.araujo, brian.curtin, esam
2010-01-29 19:15:40floxsetmessageid: <1264792540.44.0.973678542855.issue7092@psf.upfronthosting.co.za>
2010-01-29 19:15:39floxlinkissue7092 messages
2010-01-29 19:15:38floxcreate