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. A few other comments as we discussed in irc: dir(sys.implementation) should return something useful, not traceback There are a few PEP 7 violations, e.g. brace placements that should be fixed 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. 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. 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). 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. 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. 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. 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. 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! ---------- _______________________________________ 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
