Code review comment for lp:~maria-captains/bzr/bugtracker

On Mon, 2011-03-21 at 12:32 +0000, Sergei wrote:
> On Mar 21, Jelmer Vernooij wrote:
> > bzr uncommit only really changes the branch tip, it doesn't change the
> > working tree or the repository. Either way, an exception being raised
> > at a certain point should never lead to data loss or corruption,
> > especially for things that are just used for display. If you can still
> > reproduce the corruption issue, please file a bug.
> this needs uncommit to be aborted in a specific place, which cannot be
> done without modifying the code (bzr or bzr-gtk, or a special plugin).
> I don't know whether it'll qualify as a valid bug report.
Even if uncommit crashes it should not ever leave a broken tree around,
as it has no business touching the working tree.

> > > As you like. I can change it back, return a tuple everywhere, and
> > > join it into a string in the bzr-gtk. Should I?
> > Please do - at the moment the only caller is the bzr command line UI
> > which indeed uses a colon, but other callers may not. Related to this,
> > what do you think about returning the bug tracker instance and not just
> > the abbreviation? That way the caller can decide what from the bug
> > tracker to use, and they don't have to do any additional lookups.
> I cannot return a tracker instead of an abbreviation.
> Because the abbreviation is not a property of the base BugTracker class,
> every sub-class implements it differently and in a general case I cannot
> find the abbreviation, if given a tracker only.

> But if you'd like I can change this method to return a tuple (tracker,
> abbreviation, bug_id). Or I can change the BugTracker class to provide
> the abbreviation.
Ah, that's a good point. I think it would be most sensible to return
both the tracker and the abbreviation.

I agree that the way the BugTracker class works at the moment is a bit
unusual so it would benefit from some refactoring, but perhaps we should
keep that separate to prevent this branch from becoming too large and
hard to merge.

> > You should probably also add a couple of tests for lookup_id_from_url.
> > If I can help with this, please let me know.
> Does that mean that you agree with the patch in principle and only wants
> the details to be polished? Then I'll need to add the tests, and add
> lookup_id_from_url to other bugtracker classes.
Yeah, I like what you've done to allow looking up ids by URL.

> Note that reverting the url is not the only possible solution. Bzr
> could, for example, store the original bug id in the changeset, together
> with or instead of a bug url. Then there would be no need to revert the
> url back to bug id.
That requires everybody to have the exact same configuration, which is
one of the things that we've tried to avoid by using URLs. Doing these
kinds of lookups seems perfectly reasonable to me.

Cheers,

Jelmer