ease contributing with pull requests?

[Tim Graham]

Hi,

I wonder if there's any objection among the Trac team for moving toward accepting pull requests on GitHub? Is there a reason you prefer attaching patches to tickets instead? Django used to only accept patches that way, but since we started accepting pull requests some years ago, anytime someone uploads a patch to Trac, I ask if they can create a pull request instead. The ability to leave inline comments makes code review much easier and we also have continuous integration with Jenkins to automatically run the tests on each patch.

I've thought of trying to setup continuous integration for Trac using Travis unless there's already a system in place that I don't know about or if someone knows of a reason why Travis wouldn't work. I'd like to help with the Python 3 effort soon, but I think we'll need to use GitHub for review and it would be nice to have automated tests too.

Thanks, Tim

[RjOllos]

You are welcome to fork the GitHub mirror and provide your patches through a branch in your forked repository. That will allow us to use GitHub's commenting mechanism as well. Several contributors (0) have used that process and it's the process I used before I was given write access. I need to update TracDev/SubmittingPatches (1) to describe the process.

Trac runs tests on TravisCI and AppVeyor (2). I push my changes to my forked repository on GitHub to run the builds (3).

The only part of your suggestions which we don't already use is the GitHub pull requests module. To use pull requests, we'd either need to host our repository on GitHub, or implement 2-way synchronization rather than simple mirrors. I've suggested we could do that latter using SubGit, but I haven't found time to set it up. Or we could just move our repository from SVN to Git. I like the idea of using SubGit. I've used it elsewhere and found it to be very robust.

- Ryan

(0) https://trac.edgewall.org/ticket/7628#comment:24

(1) https://trac.edgewall.org/wiki/TracDev/SubmittingPatches

(2) https://trac.edgewall.org/wiki/TracDev/AutomaticBuilds

(3) https://travis-ci.org/rjollos/trac/builds

 

[Tim Graham]

Great, thanks for the info. About pull requests, I was thinking that you could create them in the mirror's queue (https://github.com/edgewall/trac/pulls), it's just that a commiter must merge the PR locally similar to what's described here: https://docs.djangoproject.com/en/dev/internals/contributing/committing-code/#handling-pull-requests

[RjOllos]

On Tuesday, August 23, 2016 at 1:58:43 PM UTC-7, Tim Graham wrote:

Great, thanks for the info. About pull requests, I was thinking that you could create them in the mirror's queue (https://github.com/edgewall/trac/pulls), it's just that a commiter must merge the PR locally similar to what's described here: https://docs.djangoproject.com/en/dev/internals/contributing/committing-code/#handling-pull-requests

It's worth considering. However, unless we developed a plugin or feature that could present those PRs in the ticket view, I tend to think it would probably just be more effort to manage the PRs since we don't currently do any work from GitHub.

This caught my attention when reviewing the Django guidelines:

"Since you can’t push to other contributors’ branches, comment on the pull request “Merged in XXXXXXX” (replacing with the commit hash) after you merge it. Trac checks for this message format to indicate on the ticket page whether or not a pull request is merged."

I was looking for an example of that, but couldn't find one after reviewing a few dozen tickets on the Django site. Could you point me to an example?

Thanks,

- Ryan

[Tim Graham]

Yes, it's some effort to keep the state of tickets and pull requests in sync. We have a "Has patch" field on our tickets as well as flags like "Patch needs improvement", "Needs documentation", "Needs tests". The ticket review queue is the tickets that have "Has patch" checked without any of the "Needs improvement" flags. After I review a pull request, I'll check the "Needs improvement" flag and ask the contributor to uncheck it after they update the PR.

Look at https://code.djangoproject.com/ticket/27089 and search for "Pull requests:" The code that generates that is https://github.com/django/code.djangoproject.com/blob/669bc946bdd378fa6e086acb3e8af80bfd319eb9/trac-env/htdocs/tickethacks.js#L99-L208

[RjOllos]

On Tuesday, August 23, 2016 at 6:06:47 PM UTC-7, Tim Graham wrote:

Yes, it's some effort to keep the state of tickets and pull requests in sync. We have a "Has patch" field on our tickets as well as flags like "Patch needs improvement", "Needs documentation", "Needs tests". The ticket review queue is the tickets that have "Has patch" checked without any of the "Needs improvement" flags. After I review a pull request, I'll check the "Needs improvement" flag and ask the contributor to uncheck it after they update the PR.

Look at https://code.djangoproject.com/ticket/27089 and search for "Pull requests:" The code that generates that is https://github.com/django/code.djangoproject.com/blob/669bc946bdd378fa6e086acb3e8af80bfd319eb9/trac-env/htdocs/tickethacks.js#L99-L208

That looks like something we could use in Trac if we were setup to merge Pull Requests on GitHub. I'll keep that in mind if we get the 2-way repository synchronization setup that would allow us to merge pull requests from GitHub.

It looks like that pull requests query through the GitHub API must run every time a Trac ticket page is loaded, but the page load time is still very good. Impressive!

- Ryan