From: Damon M. <dam...@gm...> - 2012-10-13 23:16:45
|
I probably should have tested the waters first, but I added a PEP8 github label. It's neon orange so you can't miss it. The reason I did this is so that the list of v1.2.x milestoned issues can be easily filtered by eye. That way it looks less daunting (since PEP8 isn't a huge priority for version 1.2 (at least not compared to bug fixes)) and it's easier to see the more important issues. It's also temporary. Nelle's done a great job trawling through the codebase and raising some important points. There's a finite amount of bulk work to do and then the label can be removed. If anyone is offended by neon orange, please feel free to change it. I just wanted to be able to organise things a little more succinctly. -- Damon McDougall http://www.damon-is-a-geek.com B2.39 Mathematics Institute University of Warwick Coventry West Midlands CV4 7AL United Kingdom |
From: Eric F. <ef...@ha...> - 2012-10-13 23:29:44
|
On 2012/10/13 1:16 PM, Damon McDougall wrote: > I probably should have tested the waters first, but I added a PEP8 > github label. It's neon orange so you can't miss it. The reason I did > this is so that the list of v1.2.x milestoned issues can be easily > filtered by eye. That way it looks less daunting (since PEP8 isn't a > huge priority for version 1.2 (at least not compared to bug fixes)) > and it's easier to see the more important issues. > > It's also temporary. Nelle's done a great job trawling through the > codebase and raising some important points. There's a finite amount of > bulk work to do and then the label can be removed. > > If anyone is offended by neon orange, please feel free to change it. I > just wanted to be able to organise things a little more succinctly. > I don't care about the color, but I don't understand the rationale for putting these PEP8 changes in v1.2.x, especially when they are based on master. It seemed to me that the thing to do was get out a v1.2 release without the PEP8 changes, and let them be the basis in master for proceeding to v1.3. Evidently I was wrong, and we are instead doing massive cherry-picking from master. A disadvantage of this is that PEP8 changes, though seemingly innocuous, could introduce subtle bugs, so rushing them in at the end of the rc cycle seems like it is taking an unnecessary risk for no functional benefit. Eric |
From: Damon M. <dam...@gm...> - 2012-10-13 23:52:16
|
On Sun, Oct 14, 2012 at 1:29 AM, Eric Firing <ef...@ha...> wrote: > On 2012/10/13 1:16 PM, Damon McDougall wrote: >> I probably should have tested the waters first, but I added a PEP8 >> github label. It's neon orange so you can't miss it. The reason I did >> this is so that the list of v1.2.x milestoned issues can be easily >> filtered by eye. That way it looks less daunting (since PEP8 isn't a >> huge priority for version 1.2 (at least not compared to bug fixes)) >> and it's easier to see the more important issues. >> >> It's also temporary. Nelle's done a great job trawling through the >> codebase and raising some important points. There's a finite amount of >> bulk work to do and then the label can be removed. >> >> If anyone is offended by neon orange, please feel free to change it. I >> just wanted to be able to organise things a little more succinctly. >> > > I don't care about the color, but I don't understand the rationale for > putting these PEP8 changes in v1.2.x, especially when they are based on > master. It seemed to me that the thing to do was get out a v1.2 release > without the PEP8 changes, and let them be the basis in master for > proceeding to v1.3. > > Evidently I was wrong, and we are instead doing massive cherry-picking > from master. > > A disadvantage of this is that PEP8 changes, though seemingly innocuous, > could introduce subtle bugs, so rushing them in at the end of the rc > cycle seems like it is taking an unnecessary risk for no functional benefit. > > Eric The downside of merging them into master without cherry-picking into 1.2 is the horrific merge conflicts that will occur whenever there's a pull request based on 1.2. I see your point of introducing subtle bugs. I'm happy to wait on the PEP8 changes if it seems too risky. -- Damon McDougall http://www.damon-is-a-geek.com B2.39 Mathematics Institute University of Warwick Coventry West Midlands CV4 7AL United Kingdom |
From: Eric F. <ef...@ha...> - 2012-10-14 00:24:03
|
On 2012/10/13 1:52 PM, Damon McDougall wrote: > On Sun, Oct 14, 2012 at 1:29 AM, Eric Firing <ef...@ha...> wrote: >> On 2012/10/13 1:16 PM, Damon McDougall wrote: >>> I probably should have tested the waters first, but I added a PEP8 >>> github label. It's neon orange so you can't miss it. The reason I did >>> this is so that the list of v1.2.x milestoned issues can be easily >>> filtered by eye. That way it looks less daunting (since PEP8 isn't a >>> huge priority for version 1.2 (at least not compared to bug fixes)) >>> and it's easier to see the more important issues. >>> >>> It's also temporary. Nelle's done a great job trawling through the >>> codebase and raising some important points. There's a finite amount of >>> bulk work to do and then the label can be removed. >>> >>> If anyone is offended by neon orange, please feel free to change it. I >>> just wanted to be able to organise things a little more succinctly. >>> >> >> I don't care about the color, but I don't understand the rationale for >> putting these PEP8 changes in v1.2.x, especially when they are based on >> master. It seemed to me that the thing to do was get out a v1.2 release >> without the PEP8 changes, and let them be the basis in master for >> proceeding to v1.3. >> >> Evidently I was wrong, and we are instead doing massive cherry-picking >> from master. >> >> A disadvantage of this is that PEP8 changes, though seemingly innocuous, >> could introduce subtle bugs, so rushing them in at the end of the rc >> cycle seems like it is taking an unnecessary risk for no functional benefit. >> >> Eric > > The downside of merging them into master without cherry-picking into > 1.2 is the horrific merge conflicts that will occur whenever there's a > pull request based on 1.2. To clarify, you are referring to what would happen if we did not cherry-pick, when bug-fixes are made in 1.2, and then 1.2 is merged into master? My preference would be to minimize this problem by moving quickly on 1.3, with *very* few changes in 1.2.x after the release--just quick fixes of critical bugs, if any, for a quick followup release, if necessary. In that case there are very few changes that need to be merged from 1.2.x into master, hence minimal merge conflicts. Part of my puzzlement is why, if the strategy is to get all the PEP8 changes into 1.2, Nelle hasn't been asked to base them on 1.2.x, so they could be merged from there into master in the usual way, by merging in the whole branch. It seems to me that cherry-picking from master into 1.2 should be something one does rarely, to fix an error of judgment as to which branch a change should go to, not something that should be done routinely with a whole series of PRs. > > I see your point of introducing subtle bugs. I'm happy to wait on the > PEP8 changes if it seems too risky. > That would be my preference; but has Mike written anything about where PEP8 changes should go? Eric |
From: Damon M. <dam...@gm...> - 2012-10-14 10:17:19
|
On Sun, Oct 14, 2012 at 2:23 AM, Eric Firing <ef...@ha...> wrote: > That would be my preference; but has Mike written anything about where PEP8 > changes should go? All I can find is this: https://github.com/matplotlib/matplotlib/pull/1153 -- Damon McDougall http://www.damon-is-a-geek.com B2.39 Mathematics Institute University of Warwick Coventry West Midlands CV4 7AL United Kingdom |
From: Eric F. <ef...@ha...> - 2012-10-14 18:00:49
|
On 2012/10/14 12:17 AM, Damon McDougall wrote: > On Sun, Oct 14, 2012 at 2:23 AM, Eric Firing <ef...@ha...> wrote: >> That would be my preference; but has Mike written anything about where PEP8 >> changes should go? > > All I can find is this: https://github.com/matplotlib/matplotlib/pull/1153 > Thanks. It's better than nothing, but hardly crystal-clear as guidance for the present situation. Mike's idea, as a compromise, was to "put some cleanup things (such as this) prior to the RC" but after creation of the branch. It is now well past rc3. It seems to me that putting such massive changes in now means that we should get them all in, then have another RC, and then wait a while. In addition, if this is to be the course of action, I think it would be better to rebase each PR against v1.2.x prior to review and merging so that we can be inspecting exactly the changes that will be made to v1.2.x, instead of cherry-picking. I did ask Mike earlier about cherry-picking backwards versus merging maintenance into master, and he confirmed that the latter remained the normal practice. My first choice would still be to not put any of the PEP8 things in 1.2.x, and absolutely minimize future changes on 1.2.x, freezing it ASAP. Eric |