request for code review: updates to Sankey class

Development matplotlib-devel
1

Hello,

I made a few minor changes to the Sankey class.  They are listed at:

[https://github.com/kdavies4/matplotlib/compare/master...sankey5](https://github.com/kdavies4/matplotlib/compare/master...sankey5)

Please review this and let me know if I can submit a pull request.

Thanks.

Kevin
2

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

···

On Tue, Oct 16, 2012 at 2:55 PM, Kevin Davies <kdavies4@...149...> wrote:

Hello,

I made a few minor changes to the Sankey class. They are listed at:
Comparing master...sankey5 · kdavies4/matplotlib · GitHub

Please review this and let me know if I can submit a pull request.

Thanks.

Kevin

--
Damon McDougall
http://www.damon-is-a-geek.com
B2.39
Mathematics Institute
University of Warwick
Coventry
West Midlands
CV4 7AL
United Kingdom

3

Thanks for your comments! see below...

Hello,

I made a few minor changes to the Sankey class. They are listed at:
Comparing master...sankey5 · kdavies4/matplotlib · GitHub

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.

···

On 10/16/2012 10:14 AM, Damon McDougall wrote:

On Tue, Oct 16, 2012 at 2:55 PM, Kevin Davies <kdavies4@...149...> wrote:
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