On Tue, Aug 24, 2010 at 7:52 AM, Stefan Behnel <[email protected]> wrote: > 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.
Done. >> On the Rietveld code review, there's a list of patches I've worked on: >> >> http://codereview.appspot.com/user/Haoyu%20Bai > > Ah, nice. Great work! >>> 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) Python 3 accepts a bare ellipsis, so I think we can leave it in. >>> 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. +1 >> 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. I think typeof() is the most explicit and doesn't depend on implementation details. >> 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. +1 to both of these. I'm also very excited about http://codereview.appspot.com/1943048/ . Our pure mode documentation is in huge need of an update as well. Given that it's user facing, I wonder if some of that could be stored in docstrings on the Shadow.py functions. - Robert _______________________________________________ Cython-dev mailing list [email protected] http://codespeak.net/mailman/listinfo/cython-dev
