Paul Richards
2014-08-17 10:19:49 UTC
I think we need to look at updating our wiki steps on 'how to get involved'
to make them clearer, so that people post their ideas to the mailing list
*before* starting work.
We've got comments in the
http://www.mantisbt.org/wiki/doku.php/mantisbt:development_process page like
"For non-trivial work items: If you need immediate feedback from the core
developers, email the dev mailing list with a pointer to your issue. The
feedback should be captured directly in the issue rather than the mailing
list." and "Adding or removing an external library should be discussed first
on the developer mailing list.", but they are hidden away in the text of the
page.
If you look back over time, we've had a number of people come along with
different 'patches' to Mantis to enhance something that have never been
merged as they've not been discussed/thought out first. Equally, we've got
stuff that's been done as a 'plugin' that I'd probably argue should have
been a patch to the core project.
I think we need to create a clear wiki page that tries to outline when to
create a plugin, when to create a patch, and how to discuss a new
feature/work item with the list - so it can be decided whether it should go
into a plugin, or be done as part of the core.
If you take the design's we've had over the years - we have had patches to
enhance the design and users releasing their own themes/'forks' - some of
which are pretty good, others pretty poor - so the quality varies. In all
cases, they've been done without people going to the development list first,
and nothing has ever been merged. And in some cases, never hitting the bug
tracker, forums or mailing list.
For example Mantis Themes - On github,
https://github.com/bueltge/MantisBT-Colorized /
https://github.com/J-Rabe/MantisBT-Colorized and
https://github.com/TimPietrusky/mantisbt-is-a-rockstar, and you can find
similar patches on bugs on the bug tracker.<sidenote>I actually prefer some
of these older theme's over the current 'modern' theme (and prefer some
parts of the new modern theme over the older attempts)</sidenote>
I suggest we create a single page for new developers with the following
content points expanded on:
* Thank people for their interest in mantis
* Explain that mantis has both core code + plugins
* State if making a plugin to have a think whether what you are
implementing is something generic (that might make sense to put in core) or
something specific that should likely be a plugin. If plugin, code it. If
unsure, email dev list for input.
* If implementing a trivial bug fix for core code, it's probably
just worth implementing.
* If planning a new feature, contact the dev list so advice on
historical information (relating to that feature) can be provided, and
equally, we can decide if it's a feature that would be better implemented by
adding some plugin hooks etc
* If planning to use a new library, email dev list early on so a
decision can be made.
* Provide a single patch at the end that can be reviewed easily i.e.
PR should really only have a single DIFF in it.
Let me expand on the last point briefly, when it comes down to reviewing a
PR, commits should really be collapsed in a sensible way. As I think this
probably needs some thought.
When we used to have SVN/GIT before we started doing PR's, I'd always review
a patch on its own merit. So for an example, if I look at
https://github.com/mantisbt/mantisbt/pull/235/commits - that's started off
as 1 Patch that needed a review, and now contains 3 follow up patches. In
that case all the patches are unique.
I've been tending to rebase existing patches when updating patches (in some
cases), so that I can run git format-patch on the patch, and have a proper
look through at what I'm changing as a final step (to ensure there's no
'vardump's etc left in).
And here comes before a benefit and negative with git on this - when
reviewing changes on their web interface, a single diff on a PR is the ideal
starting point. In theory, that single diff should be the complete
bugfix/feature (that should have already been discussed on the mailing list
if needed), and therefore the branch feeding it in can be collapsed into a
single commit before making the initial Pull Request. If the feature is
complex enough to warrant it, it may be worth keeping around a copy of the
branch before the commits get merged in case it aids the reviewer to be able
to look back and see how the commit got to its final point, but we don't
need to be reviewing the history on the PR.
It also allows a quick check to ensure that no whitespace changes etc have
crept in to the commit.
I'd then argue that depending on the complexity of the patch, a decision
should be made whether to do any follow up fixes as amendment to the
original patch or as a separate commit - and depending on how many people
have reviewed it. So for example, a missing semi-colon spotted by the first
reviewer should probably be an amendment. A new config option that's been
added because people figured it would be necessary should probably be a
separate commit. This allows people to review the new change only rather
than going over old work.
Thoughts?
Paul
to make them clearer, so that people post their ideas to the mailing list
*before* starting work.
We've got comments in the
http://www.mantisbt.org/wiki/doku.php/mantisbt:development_process page like
"For non-trivial work items: If you need immediate feedback from the core
developers, email the dev mailing list with a pointer to your issue. The
feedback should be captured directly in the issue rather than the mailing
list." and "Adding or removing an external library should be discussed first
on the developer mailing list.", but they are hidden away in the text of the
page.
If you look back over time, we've had a number of people come along with
different 'patches' to Mantis to enhance something that have never been
merged as they've not been discussed/thought out first. Equally, we've got
stuff that's been done as a 'plugin' that I'd probably argue should have
been a patch to the core project.
I think we need to create a clear wiki page that tries to outline when to
create a plugin, when to create a patch, and how to discuss a new
feature/work item with the list - so it can be decided whether it should go
into a plugin, or be done as part of the core.
If you take the design's we've had over the years - we have had patches to
enhance the design and users releasing their own themes/'forks' - some of
which are pretty good, others pretty poor - so the quality varies. In all
cases, they've been done without people going to the development list first,
and nothing has ever been merged. And in some cases, never hitting the bug
tracker, forums or mailing list.
For example Mantis Themes - On github,
https://github.com/bueltge/MantisBT-Colorized /
https://github.com/J-Rabe/MantisBT-Colorized and
https://github.com/TimPietrusky/mantisbt-is-a-rockstar, and you can find
similar patches on bugs on the bug tracker.<sidenote>I actually prefer some
of these older theme's over the current 'modern' theme (and prefer some
parts of the new modern theme over the older attempts)</sidenote>
I suggest we create a single page for new developers with the following
content points expanded on:
* Thank people for their interest in mantis
* Explain that mantis has both core code + plugins
* State if making a plugin to have a think whether what you are
implementing is something generic (that might make sense to put in core) or
something specific that should likely be a plugin. If plugin, code it. If
unsure, email dev list for input.
* If implementing a trivial bug fix for core code, it's probably
just worth implementing.
* If planning a new feature, contact the dev list so advice on
historical information (relating to that feature) can be provided, and
equally, we can decide if it's a feature that would be better implemented by
adding some plugin hooks etc
* If planning to use a new library, email dev list early on so a
decision can be made.
* Provide a single patch at the end that can be reviewed easily i.e.
PR should really only have a single DIFF in it.
Let me expand on the last point briefly, when it comes down to reviewing a
PR, commits should really be collapsed in a sensible way. As I think this
probably needs some thought.
When we used to have SVN/GIT before we started doing PR's, I'd always review
a patch on its own merit. So for an example, if I look at
https://github.com/mantisbt/mantisbt/pull/235/commits - that's started off
as 1 Patch that needed a review, and now contains 3 follow up patches. In
that case all the patches are unique.
I've been tending to rebase existing patches when updating patches (in some
cases), so that I can run git format-patch on the patch, and have a proper
look through at what I'm changing as a final step (to ensure there's no
'vardump's etc left in).
And here comes before a benefit and negative with git on this - when
reviewing changes on their web interface, a single diff on a PR is the ideal
starting point. In theory, that single diff should be the complete
bugfix/feature (that should have already been discussed on the mailing list
if needed), and therefore the branch feeding it in can be collapsed into a
single commit before making the initial Pull Request. If the feature is
complex enough to warrant it, it may be worth keeping around a copy of the
branch before the commits get merged in case it aids the reviewer to be able
to look back and see how the commit got to its final point, but we don't
need to be reviewing the history on the PR.
It also allows a quick check to ensure that no whitespace changes etc have
crept in to the commit.
I'd then argue that depending on the complexity of the patch, a decision
should be made whether to do any follow up fixes as amendment to the
original patch or as a separate commit - and depending on how many people
have reviewed it. So for example, a missing semi-colon spotted by the first
reviewer should probably be an amendment. A new config option that's been
added because people figured it would be necessary should probably be a
separate commit. This allows people to review the new change only rather
than going over old work.
Thoughts?
Paul