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: 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. 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? 3) Inline with my PEP8 remark in 2) above. You have some lines (maybe only one or two) that look longer than 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 -- Damon McDougall http://www.damon-is-a-geek.com B2.39 Mathematics Institute University of Warwick Coventry West Midlands CV4 7AL United Kingdom ------------------------------------------------------------------------------ Don't let slow site performance ruin your business. Deploy New Relic APM Deploy New Relic app performance management and know exactly what is happening inside your Ruby, Python, PHP, Java, and .NET app Try New Relic at no cost today and get our sweet Data Nerd shirt too! http://p.sf.net/sfu/newrelic-dev2dev _______________________________________________ Matplotlib-devel mailing list Matplotlib-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
