Haoyu Bai, 24.08.2010 16:11: > I'm not able to assign tickets to me on the trac. Could you (or > Robert?) give me the proper right to do that? Thanks!
I'm not sure which rights you need for that, so I'll leave that to Robert. > On the Rietveld code review, there's a list of patches I've worked on: > > http://codereview.appspot.com/user/Haoyu%20Bai Ah, nice. >> From a quick look, I think #488 (ellipsis) is safe, but I'd like to see some >> more syntax tests - I bet there are some in Python's own test suite. > > Looking into Python's test suite, one potential issue is Python > recognize ". .." as syntax error while my implementation will still > accept it as Ellipsis. Could we tolerate this kind of divergent? IMHO, no. If Python rejects it and it's not Cython specific syntax, we should reject it as well. (also, when compiling .py files, any non-Python syntax should be rejected) >> Ticket #487 (multiple 'with' statements) looks nice, but clearly lacks test. >> I can't even see a test that makes sure the context managers are executed in >> the correct nesting order, neither can I see anything that tests the >> chaining of cleanup actions during exception propagation, let alone partial >> propagation in cases where one of the context managers swallows an >> exception. Again, Python's test suite will provide hints on better tests >> here. > > Thanks. I just didn't aware there are so many aspects should be > tested. I'll learn from Python's tests. > > Craig also pointed out the lacking of tests in the code review. I'll > get the problem fixed. Great. Many of those tests would have been nice to have before as they also apply to nested with statements, but now that we have a proper nesting syntax, this absolutely needs tests. >> It seems you also have a fix for #477. That would be another candidate for >> 0.13.1. Note that the related test case doesn't actually test that the >> argument typing has the expected effect. This could be done using a tree >> assertion based on coercion nodes. > > The fix for #477 is part of the big pure Python enhancement patch. > Would it be worth to extract it out as a smaller patch for just this > issue? I'm not a big fan of large cumulative patches. Smaller feature patches are easier to review and apply. That allows them to go in earlier, so they can start to receive user side testing. > And yes, to test argument typing what I did is to test whether > the coercion is done by, eg. feed in the function an integer and then > see if it returns a float. Checking for the coercion node is a better > idea. I guess use the typeof() should also work? I think so. > The other patches that could be considered to be included in the 0.13.1 > release: > > #422 - bug in setting __module__: This is just bug-fixing. So should > be safe to be included. I didn't give it a review yet but it looks like a suitable candidate. > #423 - Support explicit exception chaining syntax (PEP 3134): This may > also lack of sufficient tests. > > #542 - Support for relative import: I'm not sure about this. Should be > able to be included since the syntax is not valid before. Both are pure feature additions, so, yes, if the code is ok, merging them into 0.13.1 should be fine. Stefan _______________________________________________ Cython-dev mailing list [email protected] http://codespeak.net/mailman/listinfo/cython-dev
