astitcher commented on pull request #318: URL: https://github.com/apache/qpid-proton/pull/318#issuecomment-854956723
I'm really excited to see someone working to make the python bindings based on cffi rather than swig. I think this will bring significant benefits to Proton in build simplicity and dependencies. Looking at these changes I have a few comments: 1. There seem to be many gratuitous spacing changes, which really detract from being able to see what is really changed to support the move to CFFI and things that are arguably a matter of taste or at least non functional. I'm not at all against these changes I just think it's important to separate them so that the actual functional changes are 'front and centre' and can be celarly reviewed. 2. I think the layering in this code is wrong: I think it is a requirement to keep the existing `pn_..` interfaces unchanged or as unchanged as possible. I think that the cffi binding should be a drop in replacement for `import cproton` as there a number of places where this is used: Not least currently to test the c code itself. This means I expect minimal changes to the existing python code which imports cproton and uses it. I definitely don't expect `ffi `types (`ffi.null` for example) to be present in the major python binding code. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
