Expediting Platform Pull Request Handling

[Chad Windnagle]

Quickly handling of Pull Requests

Hello Platform Maintainers:

I’m noticing that currently the platform has about 55 open pull requests. This is great! It means that many developers are finding it easy to contribute to the platform. They have great ideas and they want to share them! Moving to github has made this really easy, and I’m sure many developers are grateful that the platform is now in a place that allows this kind of collaboration!

That said, although we have 55 pull requests, only 20 of them have received any comments or interaction in the last month. 35 pull requests haven’t received any comments in the last 30 days. There’s reasons for this, of course. Either there are errors or the developer has been asked to change their code around in some way. That’s perfectly acceptable.

However, if you start to dig into the PRs, you’ll see that a lot of times the developer has responded and is awaiting more feedback from the maintainers, or there’s just not a good reason to not merge the request.

So, I’d like to open a discussion that examines different possibilities that can improve this. So, consider the floor open for any suggestions on how to expedite handling of the open requests. In the meantime, please consider an idea that was brought up on twitter. Also, in addition to this idea, please tell us [developers] what we can do to help the maintainers right now with pull requests. If there’s anything that can be done on the developer end, let us know! It’s our desire to make the maintainer’s lives as easy as possible.

An idea

With the idea of making the maintainer’s lives as easy as possible, here’s an idea that Matt Thomas brought up on Twitter. Perhaps we can create a volunteer position to help comb through and handle pull requests.

I think this is a reasonable idea. The biggest concerns that I can forsee is having that individual aware of all the ‘wants and desires’ the maintainers have for the platform. Reviewing the pull requests, I can see a lot of effort goes into understand what a code change does and proposes, and what kind of issues that might bring up. Basically, this person has to be pretty in touch with the platform project.

Alright - so please, discuss the state of things and the solution above, and any other solutions that you can come up with!

Thanks for your time, looking forward to hearing back from you!

-Chad Windnagle

[Louis Landry]

So first off, good on you Chad for diving into something like this.  I have several thoughts on the topic, and it's somewhat late so I doubt I'll get through them all in a coherent manner.  Hopefully we hear from others as well.

I'll kick it off by reacting to the suggestion on twitter.  I don't think what was suggested is a bad idea, but I'm really unsure why it is necessary to have a designated position for something like that.  It would seem to me that truly any developer who is interested can chime into the conversation on pull requests and discuss things without being granted a hat to wear.  

Perhaps it takes a hat for someone to feel like they can/should jump in and talk with other developers about their pull requests... if so I think that is sad.  We need to find ways to fight that cultural stigma.  I'm open to suggestions.  Maybe I'll start with:

I cordially invite all Joomla developers to participate in discussions on pull requests to the Joomla Platform on GitHub.  Please join in the conversation and have your voice heard.  You can make a difference just by interacting with your fellow developers.  Great developers love to be challenged by others and have in-depth conversations about code ... whether they get their way or not ... whether they are right or wrong ... and they always learn something from the discussion.  Don't wait for "maintainers" to have 1 to many discussions on things, get in there and be the community you believe we need.  

As an existing maintainer, I am much more likely to look at a pull request that people are discussing because I know there is public interest for it.  That doesn't mean I'll get to everything that people are discussing, but I'll want to that much more... and that matters.

Additionally, with active discussions amongst developers we as maintainers can more quickly and accurately build trust amongst other devs we see consistently doing things well, asking the right questions, etc.  If i'm looking at a pull request that is large or complex and a couple/few people that have submitted quality code, or consistently reviewed and helped others submit quality code are saying good things about the pull request in the discussion than I don't feel like I need to dig into things quite as deep.  I might not take the time to pull the code down and play with it.  I am much more likely to just give it a quick once over for sanity sake and merge it.  To me that is how a mature developer community operates.  

Rouven is actually a fantastic example of this sort of thing.  Rouven started off sending in all sorts of pull requests against different sections of the platform.  After a while I became to understand that his stuff was good.  I didn't need to dig too deep into things.  Mind you I still challenge him when I see things that can be improved, but I trust his judgement and understanding of what we are trying to do.  Most likely I'll just glance and merge his stuff.  Turns out it got so consistent that we decided it'd be great to join us as a maintainer -- and he's been awesome.

There's always going to be lots of open pull requests ... and to be honest we are fairly picky about what we take and don't in the platform project.  I know that several of us have been incredibly busy over hte last couple of months and haven't spent as much time and energy as we have in the past.  That's gonna happen from time to time, but we have still managed to merge quite a lot of work even then... it just so happens that there's a lot more to look at.

Anyway... that's my stream of consciousness for the night.

- Louis  

[javier gómez]

Even when I totally agree with Louis, from an internationalization point of view, I would say that "a person, or persons" that act as facilitators will help a lot. For non native english speakers is difficult to explain complex ideas. There is many good developers that can build a good improvement, but don't feel like explaining it well in the pull request. A facilitator can be a person that identifies missing documentation in a pull request and help the person that do the pull to explain what he/she is contributing.

This doesn't necessarily need to be an official role or yes if other think so.

Recently there was the report of an issue in spanish in the Joomla-CMS repo: https://github.com/joomla/joomla-cms/issues/373#issuecomment-8578213. Rouven did a great job understanding spanish ^_^. Maybe we are missing good contributions.

This is maybe off-topic but I was even wondering if it will have sense to do a kind of language forks from the main repositories. Like a: Spanish fork of the platform, a French fork of the platform... localised forks with a maintainer that allow devs to pull and comunicate in their own languages. The contributions in the local forks could be pulled and translated to the main repos... just an idea. ^_^

--
Javi

[Elin Waring]

Look, I grumble a lot about the issue of non response (I am pretty sure I have the largest number of non responded to platform list posts of anybody) and time from pr to merge. It impacts me a lot and it is sometimes depressing. But I also know I'm wrong about that sometimes when it comes to PRs. People must understand that the standards are high (the platform is picky as Louis said) and it  can and should take a while to really get work done. Last night I pushed an update to a pr from a couple of months ago. Ok, I was aggravated that it wasn't taken as it was, I totally admit that. But in truth it's much, much better now, It took me a long time to write the tests and to really think about how to solve the issue  more globally rather than in the one spot I needed it to be solved for my particular use case. And the fact that it took me days to get one failing test not to fail tells me that my original approach was incomplete. Now I have to wait and see if there is anything else to do, but I already learned a lot from making it better.  I'd like to be a developer with skills and knowledge like Louis or Ian or Sam etc and that means that right now I have to push to make my code as good and smart as I can and constantly raise my game. And that in fact means it can't be quick and easy. The fact is we are not all equally skilled or experienced. In our community we are very uncomfortable with saying that out loud, but it is the simple reality. 

Overall I think the count of open issues is really a poor metric in many respects. It can be useful but it is also highly flawed since we don't have autoclosing when people don't follow up or no one else expresses support for or interest in an issue or it is no longer mergable. It also encourages spending time reminding people to close issues that are no longer relevant just to make the statistics look better which is neither fun nor highly productive.

The more important thing in my view is how many pull requests are coming in, how many merges happen, and how much discussion there is. The more pull requests that come in the better but that means actually that a lot of open issues is actually a good thing not  a bad one.   The platform has 1463 closed issues. To me that is pretty spectacular considering that none of them are language strings or css or UI or the kinds of things commonly dealt with in the cms tracker.  

 One simple observation I would make  is that we as a project are also very reluctant to say "no" to pull requests that are clearly never going to be merged, whether because they don't make sense, they are poorly coded, they are based on misunderstandings or other things. While I think it's fine to let marginal  prs just fade to the back pages, sometimes it would be good to help someone learn to say "we are not going to do this because ..." .It's easier to avoid saying no than to do it, but it probably would help to do it a bit more. Not only does it make it less frustrating for people it also helps people learn what is expected. it helps people raise their games. That is something that more than just the maintainers need to do.  We have to embrace the idea of really tough peer code review that is not personal. Not flaming people for being idiots but calmly telling them where their thinking or understanding has gone wrong.

Another simple fact is that if code comes in without tests or to an area of the code base that doesn't have tests that is very challenging. We need to get test coverage much much higher  (and cover many more cases in a lot of places with minimal coverage) in order to know if things will be broken. Personally, my observation is that at times merges are happening too quickly and that the standard that says that no new code without test coverage is going to be accepted is not completely being followed. Admittedly it's a fine line -- you don't want to hold up a clear bug fix because the  class it impacts has no tests but really ... that is how we get b/c problems.  I've said it before. If everyone on this list would work to cover 100 lines of code we would be in a much better place. i don't envy anyone who has to manually consider the ramifications of a change in one place across the whole rest of the platform.

So on that I would say, if you have a pr in waiting, go look at the coverage of the impacted class and fill in the blanks.

 I myself am guilty of sending a pr for a new method and not adding to the doc book for it, although just like testing it is a little hard to know what to do when there is not existing chapter for a package. I don't think you should have to document a whole package to do one change (just like you shouldn't have to write tests for a whole class to fix one line) but still, that's a whole other area where probably merges are a bit too quick. 

Finally, I think it should be noted that Ian, Rouven, Louis, and Christophe all have stuff sitting in the tracker and some if it has been pretty harshly critiqued and some of the prs look to me like they won't be merged in their current forms. I also like the way Louis pushed his new cache package up for comment even though it is not ready for merging.   The fact is, not every open request is really a pull request ready to be merged.

I'm not saying things shouldn't go faster, but I am saying that it's not as simple as it seems.  

Elin

[Ian]

While I think you are probably right - there are probably good contributions that may be being missed.  On the other hand, it seems a rather difficult task to wade through everything and pick those out.

Also, you said "There is many good developers that can build a good improvement, but don't feel like explaining it well in the pull request. A facilitator can be a person that identifies missing documentation in a pull request and help the person that do the pull to explain what he/she is contributing.".  I think the same thing applies - if there is somebody who wants to get involved doing this, that's great.  On the other hand, I think part of being a contributor is putting the effort in to communicate what you are doing and demonstrate what you're trying to accomplish.  Contributing to an open source project is about more than code.

I personally don't see a lot of value in doing 'language forks' from the main repositories in any sort of official way.  That being said, forking is incredibly easy with Github and there is nothing stopping a group of Russian speaking people, French speaking people, Spanish speaking people, Swahili speaking people from getting together and working together on features or bug fixes and submitting them.  I don't know though if there is a way for somebody to communicate/intermediate on pull requests/code proposals in English.

If international communities want to work on forks of the platform they are free to do that and to push code back up when appropriate.  Any group is welcome to do so - whether they are members from non-English communities, companies building apps on the platform, even user groups or whoever.  That is one of the things GIT does really well with.

Regards

[Chad Windnagle]

Hi Louis:

Thanks for taking time to respond to this. I really appreciate it!

I have to "amen" a few of the things you wrote. Particularly about changing the "who wears the hat" culture. With the project as open as it is (no sarcasm!), perhaps the idea of having "a person" go about managing PRs in some was is counter to the place we ideally wish to be.

Regarding your words on getting other developers in the community to more or less get behind and support a PR, I've noticed that this sort of thing seems to draw maintainers attention, myself. That's kind of why I've made it a habit of reading through PRs, when I see one I like I try to tweet about it and get other devs to get on board as well. It's worked a few times (JImage auto thumbs, JMail's reply-to headers). Perhaps one thing that devs need to take to doing is doing a little shameless self promotion! 

Okay, third and final amen moment: I totally agree with you that mature projects have more mature method of handling this. I believe it was in a video with Linus Torvalds where he mentions using Git for the linux project and having faith that his maintainers were up to par enough to do a good job managing the source trees for the kernal project.

--

While reading Javier's post, one thought that has struck me is that, really what we're talking about with a facilitator-type of position...IS a maintainer! Am I wrong? Perhaps what we really need is to get some more developers, like Rouven, to help actually be maintainers on the platform project. 

Unfortunately, this means we need talented developers who have the ability to do what the current maintainers do. Someday we'll get there, but it's not as an instantaneous fix as I personally would like to see. 

--

Okay so, where are we now? Do we feel like there's a way to speed up handling of the PRs, is it really a problem in the first place? Should devs start kind of promoting their code? All of these things? Hopefully there's some ways we can work smarter and not harder, here.

Regards,
Chad Windnagle
Fight SOPA

[piotr_cz]

There are few pull requests sitting on the PR tracker for some time
that are neither merged nor commented by maintainers and then It's
hard to find out what's wrong.

For example it's a shame that https://github.com/joomla/joomla-platform/pull/1470
didn't make to JP 12.2.

On the other side often contributor abandons own PR when code style
doesn't match :[

On Oct 6, 4:36 am, Chad Windnagle <drmmr...@gmail.com> wrote:
> Hi Louis:
>
> Thanks for taking time to respond to this. I really appreciate it!
>
> I have to "amen" a few of the things you wrote. Particularly about changing
> the "who wears the hat" culture. With the project as open as it is (no
> sarcasm!), perhaps the idea of having "a person" go about managing PRs in
> some was is counter to the place we ideally wish to be.
>
> Regarding your words on getting other developers in the community to more
> or less get behind and support a PR, I've noticed that this sort of thing
> seems to draw maintainers attention, myself. That's kind of why I've made
> it a habit of reading through PRs, when I see one I like I try to tweet
> about it and get other devs to get on board as well. It's worked a few

> times (JImage auto thumbs<https://github.com/joomla/joomla-platform/pull/1391>,
> JMail's reply-to headers<https://github.com/joomla/joomla-platform/pull/1564>).

> Perhaps one thing that devs need to take to doing is doing a little
> shameless self promotion!
>
> Okay, third and final amen moment: I totally agree with you that mature
> projects have more mature method of handling this. I believe it was in a
> video with Linus Torvalds where he mentions using Git for the linux project
> and having faith that his maintainers were up to par enough to do a good
> job managing the source trees for the kernal project.
>
> --
>
> While reading Javier's post, one thought that has struck me is that, really
> what we're talking about with a facilitator-type of position...IS a
> maintainer! Am I wrong? Perhaps what we really need is to get some more
> developers, like Rouven, to help actually be maintainers on the platform
> project.
>
> Unfortunately, this means we need talented developers who have the ability
> to do what the current maintainers do. Someday we'll get there, but it's
> not as an instantaneous fix as I personally would like to see.
>
> --
>
> Okay so, where are we now? Do we feel like there's a way to speed up
> handling of the PRs, is it really a problem in the first place? Should devs
> start kind of promoting their code? All of these things? Hopefully there's
> some ways we can work smarter and not harder, here.
>
> Regards,
> Chad Windnagle

> Fight SOPA <https://www.google.com/landing/takeaction/>

> >> repo:https://github.com/**joomla/joomla-cms/issues/373#**
> >> issuecomment-8578213<https://github.com/joomla/joomla-cms/issues/373#issuecomment-8578213>.

> >> Rouven did a great job understanding spanish ^_^. Maybe we are missing good
> >> contributions.
>
> >> This is maybe off-topic but I was even wondering if it will have sense to
> >> do a kind of language forks from the main repositories. Like a: Spanish
> >> fork of the platform, a French fork of the platform... localised forks with
> >> a maintainer that allow devs to pull and comunicate in their own languages.
> >> The contributions in the local forks could be pulled and translated to the
> >> main repos... just an idea. ^_^
>
> >> On 5 October 2012 02:05, Louis Landry <louis...@gmail.com> wrote:
>
> >>>  So first off, good on you Chad for diving into something like this.  I
> >>> have several thoughts on the topic, and it's somewhat late so I doubt I'll
> >>> get through them all in a coherent manner.  Hopefully we hear from others
> >>> as well.
>
> >>> I'll kick it off by reacting to the suggestion on twitter.  I don't
> >>> think what was suggested is a bad idea, but I'm really unsure why it is
> >>> necessary to have a designated position for something like that.  It would
> >>> seem to me that truly any developer who is interested can chime into the
> >>> conversation on pull requests and discuss things without being granted a
> >>> hat to wear.
>
> >>> Perhaps it takes a hat for someone to feel like they can/should jump in
> >>> and talk with other developers about their pull requests... if so I think
> >>> that is sad.  We need to find ways to fight that cultural stigma.  I'm open
> >>> to suggestions.  Maybe I'll start with:
>

> >>> *I cordially invite all Joomla developers to participate in discussions

> >>>> on pull requests to the Joomla Platform on GitHub.  Please join in the
> >>>> conversation and have your voice heard.  You can make a difference
> >>>> just by interacting with your fellow developers.  Great developers love to
> >>>> be challenged by others and have in-depth conversations about code ...
> >>>> whether they get their way or not ... whether they are right or wrong ...
> >>>> and they always learn something from the discussion.  Don't wait for
> >>>> "maintainers" to have 1 to many discussions on things, get in there and

> >>>> be the community you believe we need.*

> >>>> *Quickly handling of Pull Requests

> >>>> Hello Platform Maintainers:
>
> >>>> I’m noticing that currently the platform has about 55 open pull
> >>>> requests. This is great! It means that many developers are finding it easy
> >>>> to contribute to the platform. They have
>

> ...
>
> read more »

[Sam Moffatt]

That particular pull request can't be merged in it's present form anyway.

And it is unfortunate that code style and unit test fixes turn some
people off. We've put in tools like the pull tester to help make it
easier to validate and review problems without necessarily having to
install the tools straight off the bat (though obviously that is
encouraged) which should help improve the ability to come to
compliance. I feel we've been very lenient on the unit test
requirements that we were hoping to enforce but until we get better
coverage that's going to be hard to do.

Cheers,

Sam Moffatt
http://pasamio.id.au