[Trac-dev] 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]

Post by 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

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

Post by RjOllos

Post by 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

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

[RjOllos]

Post by 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
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

Post by RjOllos

Post by 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
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.
"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

[RjOllos]

Post by 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

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