[Merge] lp:~salgado/bzr/bug-308122 into lp:bzr

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:
> Taking this to the list...
> 
> John A Meinel wrote:
>> I believe this is because Unshelver is meant to be a UI class, and not a
>> lower level implementation class. As in, you probably can't really
>> re-use Unshelver in gui code, as it is an implementation of a text ui
>> itself.
> 
> While shelf_ui.Unshelver is meant to be a UI class, that purpose doesn't
> exclude being used by other code.  Someone (I think it was Jonathan
> Lange) complained to me that the gap between our implementation code and
> our DWIM, UI code was too large.  I think that might be an explanation
> of why some people try to reuse our command classes rather than using
> the APIs they are supposed to use.  So with the shelf_ui classes, I was
> attempting to fill in some of the gap between command classes and the
> low-level APIs.
> 
>> Which feels like we are missing the non-ui code (how *should* qunshelve
>> interact with the shelf if not via Unshelver.)
> 
> I don't know what to make of this.  The obvious answer is
> bzrlib.shelf.Unshelver, because that's what's used to implement this UI.
>    But since you must know this, it reads as a suggestion that
> bzrlib.shelf.Unshelver is inappropriate.
> 
> If you do feel that way, I would appreciate it if you would explain why
> you think it is inappropriate.  If not, could you explain what you do mean?
> 
> Aaron

So to start with, there is a bit of confusion, because there is both

bzrlib.shelf.Unshelver
and
bzrlib.shelf_ui.Unshelver

So we have to be a bit careful which class we are talking about. I'll
admit to not being 100% sure which class is active where. It would
probably be clearer if one was called UnshelverUI. I know it is in a
different file, but it seems the standard practice has been to reference
it without the module. And certainly a line like:

   unshelver = self.manager.get_unshelver(self.shelf_id)

It isn't immediately obvious which type of Unshelver is returned here.

Anyway, there are actions like "show_changes" that seem relevant for
both a gui and a text ui, that are only present on shelf_ui.Unshelver.
Looking at it, there isn't a lot of code here, so it wouldn't be a lot
of code duplication.

I think the bigger issue is that you were trying to do something new
with the shelf_ui code. Which is probably a good thing, but because it
is different from the rest of the codebase, it isn't obvious how the
pieces interact.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksy6PkACgkQJdeBCYSNAAPqVgCgqAo7fmDTfhcJSB5y8sO8JR+x
n90Anino8eFTLQ+beaXsvFb8Va+XAdgD
=aeo5
-----END PGP SIGNATURE-----