Eric Snow <ericsnowcurren...@gmail.com> added the comment: Thanks for taking the time for the review, Barry. Again, sorry I broke the review link. It shouldn't be a problem any more.
> Barry A. Warsaw <ba...@python.org> added the comment: > > I'm inclined to go with the as_simple_namespace patch. As you say, > the pro are that this is a much better fit for this use case, while > the con is that this does kind of sneak in a new type. Given that > the type is not exposed in the API, that doesn't bother me. One > other possibility would be to move all that code to sysmodule.c as > static functions, and then it definitely won't be available outside > sys. > > OTOH, I do think this could eventually be a useful type to expose, > however the current behavior of its repr filtering out _names would > have to be removed in that case. However, that's a useful filter for > sys.implementation and it doesn't bother me, since you can always > repr sys.implementation.__dict__ to get the whole thing. > > On balance, my recommendation would be to keep it the way you have > it, but perhaps mention this on python-dev, and watch the technicolor > bikeshed go psychedelic. :) I'll bring it up on python-dev. > > A few other comments as we discussed in irc: > > dir(sys.implementation) should return something useful, not traceback Not sure what I was doing to block the lookup from object, but it's working now. So...Fixed. > > There are a few PEP 7 violations, e.g. brace placements that should > be fixed Done. > > I was looking at _namespace_init() and a few things bothered me. I > thought this might be superfluous and that you'd be able to just > inline the PyDict_Update() calls, but now I'm not so sure. AFAICT, > Python's documentation is silent on whether *args and *kwds in a > tp_init function can be NULL or not, and I definitely see cases where > similar checks are made in other types. So I think with that in > mind, you must assume they *can* be NULL. But then I'm not sure the > assertions are useful or correct. Yeah, they're mostly an artifact of copying code. However, I'm glad they triggered your explanation. ;) > Take this scenario: > > namespace_init() gets args==NULL. Your assertion doesn't trigger, > but PyObject_Size(NULL) sets an exception and returns -1. Your > conditional doesn't check for an error condition in PyObject_Size() > so you'll incorrectly swallow (temporarily) that exception. At the > very least, you need to check for PyObject_Size() < 0. Don't forget > too that those assertions can get compiled away. > > I think I'd rather see a PyTuple_Size(args) conditional here for the > args parameter. If it's not a tuple, you'll get an exception set, so > check for size < 0. If size > 0, then you can set the TypeError and > return -1 explicitly. Pretty sure I understand. Let me know if my new patch says otherwise. <wink> So...Done. > > Similarly, just call PyDict_Update(kwds) and return its value. If > kwds is neither a dict nor has a .keys() method (including if its > NULL), an exception will be set so everything should work correctly > (see _PyObject_CallMethodId() in abstract.c, which is what > PyDict_Update() boils down to). Done. This was a case of prematurely optimizing. > > At least, I'm nearly certain that's safe :) > > Moving on... > > namespace_repr() also has some PEP 7 violations (brace position, > extra blank lines). Please fix these. Done. > > Is it ever possible to get into namespace_repr() with ns->ns_dict > being NULL? I think it's impossible. You might rewrite the if > (d == NULL) check with an assertion. Done. > > I think you're leaking the PyList_New(0) that you put in `pairs` > after you PyUnicode_Join() it. PyUnicode_Join() doesn't decref its > second argument and your losing the original referenced object when > you assign `pairs` to its result. Good Catch. Fixed. > > I find the mix of inline error returns and `goto error` handling in > namespace_repr() somewhat confusing. I wonder if it would be more > readable (and thus auditable) if there was consistent use of `goto > error` with more XDECREFs? If you feel like it, please try that out. I've given it a go. :) > > That's it for now. Please submit another patch for the > as_simple_namespace case and I'll take another look. Also, if you > send a message to python-dev about the introduction of the namespace > object, I'll follow up with my support of that option. > > Thanks, this is looking great! Thanks again for all your support through this process! ---------- Added file: http://bugs.python.org/file25767/issue14673_as_simple_namespace_2.diff _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue14673> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
