Trac issues review
bur...@gmail.com
On Sat, Aug 14, 2010 at 2:03 PM, Russell Keith-Magee
<rus...@keith-magee.com> wrote:
> On Sat, Aug 14, 2010 at 1:39 PM, bur...@gmail.com <bur...@gmail.com> wrote:
>> I had few useful thoughts about changing the way Django development
>> contributions gets accepted and committed -- but all I get from this
>> mailing list that all attempts to improve any process is just a waste
>> of time.
>
> That's not entirely fair.
>
> We're acutely aware of the fact that there is a backlog of tickets
> that need attention. We're also aware that many people upload patches,
> but then don't get any feedback on them. We want to improve this in
> any way we can.
>
> If you have suggestions on how we can improve Django's development
> process and address these issues, we're happy to hear them.
>
> However, that doesn't mean we're going to immediately implement a
> suggestion just because it's been made. I don't know which suggestion
> in particular you're referring to, but if you've made a suggestion and
> we haven't acted on it, then I would suggest to you that this is
> because we (the core team) didn't agree that your idea was as useful
> or practical as you think it was.
>
> If you have a suggestion, please make it. We're listening.
Ok. I think the reviewing workflow should be improved in the way core
developers, triagers and patch developers should get a list of tickets
to work on. Please correct me if I'm wrong, but I don't see any
mechanism that will allow you:
a) to find issues that needs a review (not some issues, but all issues
ordered by importance or date sent to reviewing)
b) to mark issue as reviewed.
c) to mark someone (core dev/someone else) as a curator on an issue,
so core team doesn't repeat themselves reviewing each bug multiple
times when it's not required.
The problem issues don't get attention is that no one usually know
they need their attention.
Bug states don't help a lot: they mark the maturity of solution but
not who's move is now.
And current process from my point of view is a Monte-Carlo issue
picking. It's not broken -- speed is almost the same (cause it's the
speed of commit process), but absolutely no feeling of control.
Also after the review/commit separation, we'll be able to tell
exactly, if we need more reviewers, patch writers/rewriters, core dev
team members or just committers.
>> Core devs have no time even to accept working & looking good
>> patches -- and rarely have time to review patches not looking good or
>> working wrong!
>> So why should I bother to write patches fast?
>
> I would point out that the core team aren't the only people that can
> give feedback. Trac is an open resource for exactly that reason.
Yes, this is one more point. Regular developers, like me, also don't
know what to review, and for most of patches, they just can't write a
review: they have high risk of writing wrong suggestion. Patch author
or triagers usually know if people can write a review, but people
can't find such issues.
So probably "reviewers" bugs category should also exist, but should be
also treated differently.
If we agree with such a system, I can walk through all issues
(probably with scripts), and write who needs to respond on each issue.
Patch developers usually know whether everyone wants feedback from
them, but I'd love if we were able to reduce core developers load with
introduction of such system and adding more reviewers.
Think of the following: every personal django branch on github can be
considered as adding a reviewer & committer. Why not ask them to help
with tickets? But then they need to know how they can help. We outline
issues to write patches for, but not for review.
Since tickets with needs_better_patch=1 could be both reviewed or not
reviewed, I haven't find any way to get any reviewing queue.
On implementation side, I consider two checkboxes with "[ ] needs
public review" and "[ ] needs core dev review", and a button with
"(Set me as a curator on this issue)", or maybe a text input for
curator email.
That way, DDN stage might not be needed -- it's just "unreviewed" +
"needs core dev review".
Date of last ticket update can be also added to the trac reports -- to
sort tickets based on that date.
A robot can be written to fill these fields based on other fields,
comments authors, and knowledge of who are in core dev team. I would
also consider an external service for reviewing and triaging, but that
way you won't see review states in trac.
--
Best regards, Yuri V. Baburov, ICQ# 99934676, Skype: yuri.baburov,
MSN: bu...@live.com
Jeremy Dunck
> Hi Russell,
>
> On Sat, Aug 14, 2010 at 2:03 PM, Russell Keith-Magee
> <rus...@keith-magee.com> wrote:
>> On Sat, Aug 14, 2010 at 1:39 PM, bur...@gmail.com <bur...@gmail.com> wrote:
>>> I had few useful thoughts about changing the way Django development
>>> contributions gets accepted and committed -- but all I get from this
>>> mailing list that all attempts to improve any process is just a waste
>>> of time.
>>
>> That's not entirely fair.
Sorry, I've missed some context here -- can someone point me to the
beginning of this thread?
bur...@gmail.com
Thread "#11834: colorized debug page. Assigned to buriy" (
http://groups.google.com/group/django-developers/browse_thread/thread/bd43e2e040a17784
) is the beginning.
Dougal Matthews
Jeremy Dunck
> Hi Jeremy,
>
...
>>
>> Sorry, I've missed some context here -- can someone point me to the
>> beginning of this thread?
>
> Thread "#11834: colorized debug page. Assigned to buriy" (
> http://groups.google.com/group/django-developers/browse_thread/thread/bd43e2e040a17784
> ) is the beginning.
>
Thanks, got it.
Russell Keith-Magee
That is -- all the tickets that have been accepted and have a patch
that is currently marked as "ok".
There's a separate query that also requires attention:
This is the triage task, and it's just as important as
writing/reviewing patches.
Ordering by 'importance' isn't something we support. Historically, we
did, and it consistently proved to be almost useless as a measure of
anything. Everyone thinks that the problem they submit is very
important, so having importance as an open field in Trac doesn't yield
useful data. A more helpful measure of 'importance' is the vitality of
the discussion under the ticket; a ticket with lots of interest will
almost always have a large number of comments, usually a couple of
which that say "why hasn't this been merged yet?". The other useful
measure is the number of times an issue is re-reported. We don't have
a good measure of this, AFAIK.
Ordering by date isn't something that we expose particularly well
either, but we have an analog -- the ticket number. Tickets are
allocated sequentially, so ticket 1234 was logged before 2345. It
doesn't take too long working with Trac to work out that a ticket with
a number in the 8000 range is about 2 years old; 14000 is less than a
month old.
> b) to mark issue as reviewed.
This one we definitely support. Tickets that have been reviewed and are good:
Tickets with a negative review:
> c) to mark someone (core dev/someone else) as a curator on an issue,
> so core team doesn't repeat themselves reviewing each bug multiple
> times when it's not required.
> The problem issues don't get attention is that no one usually know
> they need their attention.
> Bug states don't help a lot: they mark the maturity of solution but
> not who's move is now.
> And current process from my point of view is a Monte-Carlo issue
> picking. It's not broken -- speed is almost the same (cause it's the
> speed of commit process), but absolutely no feeling of control.
I'm not sure exactly what you're concerned about here.
The "curator" of an issue is the owner. If you claim a ticket, you are
curating it.
As for the problem of multiple reviewing: Allow me to assure you that
for me personally, 'double reviewing' of tickets isn't a problem I
face. I have three working lists for tickets:
* The list of tickets marked against milestone 1.3
* The list of tickets marked Ready For Checkin
* My personal list of tickets that I've flagged as interesting. This
usually means that something in the report caught my attention, or
because someone on django-dev has made a polite request for review.
If I review a ticket on the RFC list and it isn't RFC, it gets bumped
back to Accepted (with any other flag changes that are required)
If a ticket is on either of the other two working lists, I work on
them until they're finished (which means the ticket is either
committed, or closed wontfix/invalid etc). That's because these lists
are driven by a different set of priorities; Milestone lists because
they're standing between me and a RC; personal lists are things that
are my own itches that I want to scratch.
It shouldn't be a problem anyone else faces, either. The "problem
issues that need attention" is "any ticket that isn't currently
closed". The "who's move is now" can be determined by reading the
ticket; it's either the owner of the ticket (or the person who has
provided the patch), or the ticket needs review -- from anyone who can
volunteer the time.
If the issue is that this is an imposing list, that's because there's
lots of work to do. Adding extra triage states doesn't reduce the
amount of outstanding work -- it just diverts time that could be spent
fixing bugs into bit twiddling on trac.
If the number of tickets requiring work is imposing, I would suggest
that the 'component' flag is the best way to combat this. This allows
you to focus on a particular area (e.g., contrib.auth), which provides
focus on the tasks to be done, and reduces the overwhelming nature of
the ticket list. It also helps me -- when I sit down to commit, I
vastly prefer concentrating my efforts in a single area of code, since
it allows me to get familiar with a single area of code and fix
multiple issues in rapid succession.
> Also after the review/commit separation, we'll be able to tell
> exactly, if we need more reviewers, patch writers/rewriters, core dev
> team members or just committers.
I don't need any changes to Trac to answer this question. The answer
to this is "Yes". :-)
>>> Core devs have no time even to accept working & looking good
>>> patches -- and rarely have time to review patches not looking good or
>>> working wrong!
>>> So why should I bother to write patches fast?
>>
>> I would point out that the core team aren't the only people that can
>> give feedback. Trac is an open resource for exactly that reason.
>
> Yes, this is one more point. Regular developers, like me, also don't
> know what to review, and for most of patches, they just can't write a
> review: they have high risk of writing wrong suggestion. Patch author
> or triagers usually know if people can write a review, but people
> can't find such issues.
There's no nice way to sugar coat this. There is a lot of work that
needs to be done. We have a lot of open tickets.
The most important thing is to do *something*. Be bold -- if you
review something and you think you've go the right solution, mark it
as Ready For Checkin. If you want clarification on something before
you do this, ask on django-dev (mailing list or IRC).
Yes, some of your reviews will be wrong, especially in your first
attempts. However, allow me to assure you that if I find a patch in
Ready For Checkin that doesn't meet trunk standards, I'll push it back
to Accepted with a 'patch needs improvement flag', along with review
notes.
> So probably "reviewers" bugs category should also exist, but should be
> also treated differently.
I don't understand what you're proposing here. Again, any ticket that
isn't in RFC needs either a review or a patch. Trac already gives you
the tools to separate those two lists.
> If we agree with such a system, I can walk through all issues
> (probably with scripts), and write who needs to respond on each issue.
> Patch developers usually know whether everyone wants feedback from
> them, but I'd love if we were able to reduce core developers load with
> introduction of such system and adding more reviewers.
This is where your argument falls apart. Adding extra flags in Trac
doesn't increase the number of reviewers that we have. Sending an
email to someone doesn't compel them to do work -- this is a volunteer
project. If anything, bit twiddling on Trac *reduces* the amount of
review work that gets done, because the effort is being spent
twiddling bits rather than reviewing actual code.
What adds more reviewers is people actually doing work. I'm afraid I
don't see how increasing the complexity of the interaction in Trac
serves to increase the number of people contributing.
However, I will concede that Trac has a fairly awful UI, and there may
be room for improvement to polish our Trac install to make it easier
to consume Trac information (for example, having more prominent links
to useful Trac query reports). If Trac configuration is an area in
which you have expertise, we'd be happy for the help.
(... and to head the inevitable argument off at the pass... that isn't
an invitation to propose adopting an alternatives to Trac. *All* bug
trackers suck, they just suck in different ways. Trac sucks in ways we
understand.)
> Think of the following: every personal django branch on github can be
> considered as adding a reviewer & committer. Why not ask them to help
> with tickets? But then they need to know how they can help. We outline
> issues to write patches for, but not for review.
You appear to be under the impression that we haven't done this. I've
lost count of the number of times I've asked people to contribute.
I've also lost count of the number of times I've told people that if
they provide a git/hg branch that is a rich vein of trunk-ready fixes,
I'll merge that branch, and if they make a habit of providing
trunk-ready patches, we can see about getting trunk commit access.
> Since tickets with needs_better_patch=1 could be both reviewed or not
> reviewed, I haven't find any way to get any reviewing queue.
Agreed, there is a slight ambiguity in the 'needs-better-patch' flag.
The obvious fix is to make it a tri-state flag instead of a boolean --
* Needs Review
* Patch needs improvement
* Patch is OK.
However, this ignores the overlap with 'ready for checkin'. A ticket
that is 'ready for checkin' tells you that the patch is ok. If you
have a patch that is 'needs-better-patch=false', but it isn't in
'ready for checkin', then it hasn't been reviewed.
I will completely agree that this isn't ideal from a UX point of view
(i.e., it's logically consistent if you think about it, but the fact
you need to think about it isn't a good thing). However, we're
slightly hamstrung by what Trac can give us here.
> On implementation side, I consider two checkboxes with "[ ] needs
> public review" and "[ ] needs core dev review", and a button with
> "(Set me as a curator on this issue)", or maybe a text input for
> curator email.
"Set me as curator" already exists - it's called the ticket owner.
"Needs core-dev review" is "ready for checkin".
"Needs public review" is anything that has a patch, but isn't in
"ready for checkin", and doesn't have the 'needs improvement' flag
set.
> That way, DDN stage might not be needed -- it's just "unreviewed" +
> "needs core dev review".
No -- DDN is a different issue. DDN means that there is a larger
design issue that needs to be resolved. This might be resolved by core
developer fiat, but more likely, it requires someone to start a
discussion on django-dev.
> Date of last ticket update can be also added to the trac reports -- to
> sort tickets based on that date.
Again, I'm not sure I see the importance of this. Ticket number gives
you a sequential ordering (which lets you hit "new" issues"); beyond
that, any bug is a bug, and needs to be fixed.
> A robot can be written to fill these fields based on other fields,
> comments authors, and knowledge of who are in core dev team. I would
> also consider an external service for reviewing and triaging, but that
> way you won't see review states in trac.
To me, this points to a fairly glaring logical fallacy. If a robot can
fill in new "reviewing" fields based on existing data, then doesn't
that mean that the existing data is sufficient to determine reviewing
state?
Yours,
Russ Magee %-)
bur...@gmail.com
Thanks for clarifications of the process, but it seems I explained
some things wrong.
I see some issues have the following problems:
1) second core developer review takes a year,
2) reviewers like me who are not core developers don't know if they
will be able to say something new a ticket without reading it, and how
to find such list of tickets.
When you have not much pairs of eyeballs, it's very important to have
good tickets coverage, and if some tickets don't get reviewed in a
year, it does mean these eyeballs distribution among tickets isn't
good.
By "curator" I meant core dev team member, who got feedback last time
(or sometimes someone else who can perform core team person job). This
is different from ticket owner (yes, few years ago the ticket owner
was "curator", but now tickets are assigned to patch developer).
When I mentioned "sorting by time" I meant time of ticket last change,
not ticket creation. I explain it that we currently don't have any way
to find "stale" tickets, which are waiting for some specific action,
but no one knows what exact action.
I see it like all developers can be divided into 2 categories:
- core dev member, who can make serious decisions, and
- regular developers, who can only check if patch works, if they like
syntax, if there's no any simple logical flaw, or if patch doesn't
work for them, but they can't approve if patch solves the problem is
right way. Usually one want to know if they patch solve the problem
correctly before they polished patch and it got to RFC.
And in this decision making i see the big difference before regular
and core dev developers.
Basically I want to know if I can do something on ticket without
reading it through. If I can't -- I want to tell that I can't review
ticket and who can.
So, yes, the additions I propose can be mostly done automatic, with
few exceptions:
- ticket creator marks if issue can be reviewed by any developer or
only core dev (if decision making process is still ongoing). The only
current alternative now is django-dev mailing, I'm not sure if such
mailing should be done every time more core dev clarifications are
needed -- if it is so, I'll go through a couple of my bugs and lots of
other people bugs and start a dozen of threads here right now.
- regular developer marks if they're done with reviewing issue, and
if the only issues left are decision making ones -- transforming to
DDN again might be considered as an alternative here.
On Mon, Aug 16, 2010 at 8:28 AM, Russell Keith-Magee
<rus...@keith-magee.com> wrote:
>To me, this points to a fairly glaring logical fallacy. If a robot can
fill in new "reviewing" fields based on existing data, then doesn't
that mean that the existing data is sufficient to determine reviewing
state?
Except these two differences marked above, yes, but you can't sort
trac issues based on such derived data.
...Or maybe I'm so selfish and tend to complicate things (both is
true), and my small ticket-being-able-to-review / ticket-read ratio
should be ignored as one of the life inconveniences.
Russell Keith-Magee
> Hi Russell,
>
> Thanks for clarifications of the process, but it seems I explained
> some things wrong.
>
> I see some issues have the following problems:
> 1) second core developer review takes a year,
> 2) reviewers like me who are not core developers don't know if they
> will be able to say something new a ticket without reading it, and how
> to find such list of tickets.
>
> When you have not much pairs of eyeballs, it's very important to have
> good tickets coverage, and if some tickets don't get reviewed in a
> year, it does mean these eyeballs distribution among tickets isn't
> good.
>
> By "curator" I meant core dev team member, who got feedback last time
> (or sometimes someone else who can perform core team person job). This
> is different from ticket owner (yes, few years ago the ticket owner
> was "curator", but now tickets are assigned to patch developer).
This implies that the 'curator' won't ever change; that once a core
developer has 'claimed' a ticket, s/he will be the only person to give
feedback. Unless a patch is particularly large and messy (which
usually means "feature") this isn't the case. A review is a review.
Also -- just because I'm able to give feedback on a ticket today
doesn't mean I'll be in a position to give feedback tomorrow.
> When I mentioned "sorting by time" I meant time of ticket last change,
> not ticket creation. I explain it that we currently don't have any way
> to find "stale" tickets, which are waiting for some specific action,
> but no one knows what exact action.
Ok; this is a reasonable suggestion, and it's one that came up the
last time Trac was discussed.
The biggest impediment here is Trac itself; Trac doesn't share
Django's policy on backwards compatibility (they've changed template
languages twice in the last two releases), so we're stuck on an old,
buggy Trac install that we can't update easily.
Jeremy Dunck has been looking into these headaches recently (which, I
suspect, is why he raised his voice earlier in this thread). I'll
leave it up to Jeremy to give a status update (or call for additional
help if he needs it).
> I see it like all developers can be divided into 2 categories:
> - core dev member, who can make serious decisions, and
> - regular developers, who can only check if patch works, if they like
> syntax, if there's no any simple logical flaw, or if patch doesn't
> work for them, but they can't approve if patch solves the problem is
> right way. Usually one want to know if they patch solve the problem
> correctly before they polished patch and it got to RFC.
Well, it would be nice if only valid, trunk-ready patches made it to
RFC, but frankly, I'll take what I can get. *Any* review is better
than no review, and if a reviewer is planning to make a habit of
contributing, I would hope that regular feedback when bumping an RFC
patch back to Accepted would eventually result in less bump-backs, and
more committable tickets.
> And in this decision making i see the big difference before regular
> and core dev developers.
>
> Basically I want to know if I can do something on ticket without
> reading it through. If I can't -- I want to tell that I can't review
> ticket and who can.
Is the ticket Ready For Checkin? If not, then there is something you
can do. What needs to be done will vary from ticket to ticket, but the
other ticket flags will generally reveal whether a test is required,
or a patch needs review, or documentation is required, etc.
Who can review the ticket? Anyone who is willing and able to
contribute the time. Decisions are made by those who show up.
> So, yes, the additions I propose can be mostly done automatic, with
> few exceptions:
> - ticket creator marks if issue can be reviewed by any developer or
> only core dev (if decision making process is still ongoing). The only
> current alternative now is django-dev mailing, I'm not sure if such
> mailing should be done every time more core dev clarifications are
> needed -- if it is so, I'll go through a couple of my bugs and lots of
> other people bugs and start a dozen of threads here right now.
I'm not sure I see the value in making the distinction between "can be
reviewed by core" or "can be reviewed by the public".
A bug is a bug. If someone reports a bug, it's obviously a problem for
them. If they provide a patch, presumably it fixes the problem for
them. If another developer can reproduce the bug, and can confirm that
the patch fixes the bug, that's a significant contribution. If the
reviewer can sanity check the patch to make sure it has all the
required components, and looks like the right solution to the problem,
that's even better.
The only nebulous part in this process is whether a patch fixes a bug
in the 'right' way. This is really hard to establish, but it's one of
those things that comes with experience.
It sounds like you might be suggesting that what we need is an
intermediate step -- a way to flag that you (as a non-core
contributor) have verified that a bug exists, that the patch applies,
and that it contains all the relevant bits (tests, docs etc), but that
you're not comfortable pushing to RFC because you're not sure if the
patch is "correct" to trunk standards.
To this I'd say that the solution is to be bold. Pick a ticket where
you are moderately confident that you've got the right solution, and
push it to RFC. If you're wrong, it will shake out when a core
developer looks at it. If I start seeing a lot of RFC comments from a
particular contributor, I'll make an effort to look at their work to
make sure they're not wasting their time.
If you're not comfortable being that bold, say so in the ticket
comments; someone else might have more confidence, and if you have
gone through the preliminary steps (i.e., verifying that the bug
actually exists, that the patch fixes the bug, that the patch applies
cleanly, that the patch contains a test and docs as appropriate) then
you've provided some very useful feedback for someone else to work
with.
As for mailing django-dev: Simply flooding the list isn't going to win
you any friends. Mailing "I've reviewed ticket XYZ" isn't going to win
friends, either. However, if you have a genuine inquiry, django-dev is
exactly the right place -- point at a couple of tickets, ask for
clarification on the issue that is preventing you from pushing a
ticket to RFC (e.g., Is this patch on the right track, or should I be
looking higher up for a root cause?).
> - regular developer marks if they're done with reviewing issue, and
> if the only issues left are decision making ones -- transforming to
> DDN again might be considered as an alternative here.
DDN means that there is some complex issue that needs resolution at
the project level -- something where there are multiple possible
options or interpretations. It's not intended as a "I need a core
developer to validate this patch" flag -- that's what RFC is for.
>>To me, this points to a fairly glaring logical fallacy. If a robot can
> fill in new "reviewing" fields based on existing data, then doesn't
> that mean that the existing data is sufficient to determine reviewing
> state?
> Except these two differences marked above, yes, but you can't sort
> trac issues based on such derived data.
>
> ...Or maybe I'm so selfish and tend to complicate things (both is
> true), and my small ticket-being-able-to-review / ticket-read ratio
> should be ignored as one of the life inconveniences.
Complicating things is something I want to avoid, but if there is
something that is standing between you and being an effective
contributor, then I'd like to get to the root of that issue.
I should also point out that I'm not opposed to making changes to
Trac. As I noted before, the biggest impediment at the moment is Trac
itself; once we've got the basic install up to date, we will be in a
better position to add new features.
Personally, I have two features that I would like to see. Instead of a
adding a whole ticket "review state", I'd like to see the ability to
capture, on a per-user basis, the following:
* This bug affects me personally
* I can verify that the patch on this ticket fixes the problem for me
The goal here would be to provide a way to generate reports of:
* Bugs that are affecting the most people
* Bugs that have patches that multiple people think are good (in the
sense that they make the problem go away), but nobody is willing to
push to RFC.
In both cases, this is just quantification at a database level of the
sorts of comments that people are leaving as "+1" right now. IMHO,
exposing these +1's as formal Trac reports has the potential to
provide a rich source of feedback to reviewers and the core team,
providing a hit list of important issues.
Yours,
Russ Magee %-)