Thanks for your comments! see below... On 10/16/2012 10:14 AM, Damon McDougall wrote: > On Tue, Oct 16, 2012 at 2:55 PM, Kevin Davies <kdavi...@gmail.com> wrote: >> Hello, >> >> I made a few minor changes to the Sankey class. They are listed at: >> https://github.com/kdavies4/matplotlib/compare/master...sankey5 >> >> Please review this and let me know if I can submit a pull request. >> >> Thanks. >> >> Kevin > Thanks for taking the time to fix up a part of the codebase! > > If you make a pull request out of your code, we'll be able to leave > inline comments on your patches. Nonetheless, I have some feedback for > you after a (very) quick glance: I submitted the pull request. > 1) I don't think 'orientations' is a python keyword. What is the error > you were getting? In any case, changing it breaks backwards > compatibility so I'd be an advocate of keeping 'orientations'. Unless, > of course, the error you were getting was serious. The problem was on my end (In my code, I intercepted orientations as a named argument but assumed it being passed through **kwargs). I reverted to the original. Thanks for your patience. I'm sorry. > 2) Your changes appear to be, mainly, cosmetic. This is good but may > cause issues with some of the PEP8 pull requests we have been getting. > Have you rebased to make sure these changes are incorporated? I rebased off master after pulling from origin. That's correct, right? > 3) Inline with my PEP8 remark in 2) above. You have some lines (maybe > only one or two) that look longer than 79 characters. I re-wrapped everything to 79 characters. > Other than that, I think you should turn this into a pull request so > you can get more feedback on an interactive level. > > Best, > Damon >
------------------------------------------------------------------------------ Everyone hates slow websites. So do we. Make your web apps faster with AppDynamics Download AppDynamics Lite for free today: http://p.sf.net/sfu/appdyn_sfd2d_oct _______________________________________________ Matplotlib-devel mailing list Matplotlib-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
