Code review comment for lp:~vila/bzr/move-test-servers

Revision history for this message
Vincent Ladeuil (vila) wrote :

Cough, so to clear up a few points:
- bzrlib.transport.Server still exists
- only chroot and pathfilter are really used as servers,
- there is still various grey areas around inheriting from
  b.t.Server or just implementing {start|stop}_server(), and yes
  get_url() is only used for tests,
- I expect most fallouts from LocalUrlServer and to a lesser
  extent MemoryServer (if the bzr code base usage can serve
  as a comparison point),
- given the above, only *tests* should be failing from there.

Also, I first started to deprecate b.t.Server and it helped me
find all obscure usages, but in the end I had to leave it alone to
separate "real" server usages (chroot and pathfilter) from true test
usages. And since a server is just implementing {start|stop}_server()
I already wondered if it wasn't already too much (or said otherwise
a small enough API to not even bother having a base class for it, which
is exactly what smart_server does).

About starting a bzrlib.transport.servers a bzrlib.servers hierarchy:
- YAGNI,
- a slight preference for bzrlib.servers when needed but so far,
  I don't even consider our ftp and http servers like true servers.
  Their intent is to be hacked on to be able to test any crazy
  failure mode. As such, if there is ever a decision needed to be
  made between making them easy to customize or making them faster,
  I will chose the former.

I'll add a NEWS entry explaining the migration path (mostly,
as John said changing an import). Since the fix is trivial,
I don't think we need to make an exception for launchpad
(which may already have other API skews to fix).

Since it has been approved, I'll land that patch which is already
massive and we can improve from there.

« Back to merge proposal