tbzr code for discussion.

[Mark Hammond]

Hi John,

  Thanks for the feedback!
 
> 1) It is actually tbzrlib/pipe/client.py not "cache".

Thanks - fixed.

> 2) no win32pipe.TransactNamedPipe - consider upgrading pywin32to build
> 212
> 
> I get this when running a query. But I wasn't able to find a
> __version__ string,
> or a build string in my pywin32 install. Usually I would expect to be
> able to do:

Yeah.  Its not very pythonic, but the version numbers are built into the binaries (from Explorer, right click a .exe/.pyd/.dll from pywin32, and select Properties->Version.  Also, a pywin32.version.txt is installed in site packages - but its not exposed in a "nicer" way.

Regardless, 212 hasn't been released yet :)  TransactNamedPipe allows us to combine the write and reads into a single call, which apart from the Python overhead saved, Windows promises it will happen in a "single network operation".  This is used by the client, which will only be Python implemented in the short term, so its really not an issue - I'll just tone down the warning for now, and maybe re-enable it later (its only relevant to source-code users anyway)

> Maybe I'm just missing something. (Also, your 'license.txt' still only
> says
> 1999-2001, I assume you want copyright through 2008).

Nice spot, thanks :)  It's actually been my intent to try and relicense pywin32 user a standard PSA license, but I'm not very good at performing such tasks :)

> 3) I find it a bit odd to have the scripts in a subdirectory of the
> library.
> (Running tbzrlib/app.py imports 'tbzrlib' to run.) Maybe that is just
> me. But it
> does make running out of the working directory more difficult than it
> has to.
> (Even if you want to put it into a 'scripts/' directory, you can add a
> quick
> tree-level shim which shoves the right path into sys.path to make it
> easy to run
> from source.)

Yes, "scripts" makes sense to me.  You might notice from the code that "tbzrlib.app.get_app()" gets the singleton Application object, which is effectively the single global variable that ties the various pieces together.

Do you think it makes sense to keep the tbzrlib.app module with the the "Application" object itself, including the "run" logic, while a new file in the "scripts" directory contains not much more than the __main__ entry point?

> 4) Doing client.py file_status . gives a random error about not enough
> arguments. Doing 'client.py file_status . 1' gives an ERROR with a
> traceback. I
> assume because '.' is a directory and not a file.
> But certainly the error is confusing as the final error is:

Yes - file_status needs another arg.  Note that client.py's entry point is nothing more than a test and I expect it will be removed.

> ~    (val,) = struct.unpack("H", buf[ndx:ndx+int_size])
> TypeError: 'NoneType' object is unsubscriptable

Yeah - the client isn't expecting errors yet ;) 

> 5) If you need to run 'tbzrlib/app.py' directly, why do you need to run
> 'hello'
> as well?

See above - I need to make it clearer that those instructions are simply for developers to ensure the remote app is running and the pipe works as expected (and goodbye may end up being useful for developers once the app auto-restarts itself).

> Nesting is great for me,

Great! The Zen of Python tells us "flat is better than nested", but I (obviously:) agree that some nesting is beneficial.

> as long as the tests all chain properly.

I'm not sure what you mean by that - in general, the test don't depend on other tests, so I'm not sure what chaining refers to.

> We took the
> approach of nesting tests to mimic the library layout, but that isn't
> the only way. (Specifically, a file bzrlib/subdir/foo.py should have 
> a test as something like bzrlib/tests/subdir/test_foo.py, though we 
> don't do that as much as we probably should.)

Right - so now I have (kinda):

  tbzrlib\cache\foo
  tbarlib\cache\tests\test_foo

whereas to be consistent with bzrlib, this should look like:

  tbzrlib\cache\foo
  tbzrlib\tests\cache\test_foo

I'd be more than happy to make that change - anyone second that motion? :)

> We have ConfigObj for ini-style configuration. You would want per-user
> configuration, and since this is really win32-only, I don't really see
> why you wouldn't use the Registry.

I'm really in 2 minds here.  The registry is obviously the "windows way", but the registry doesn't seem to offer us much more than a config file in a well chosen location (so it will "roam" with the user as they login to different machines on the network).  A config file can be edited in a text file - although is much harder to update programatically if you want to keep user's comments etc in place.  On the other hand, if you are using the registry, the entire concept of comments next to preferences is moot.  We could ask users to send us their preferences file if we suspect something related to user prefs.  The registry offers transactions, fine-grained security, remote access, and much other goodness, but I don't see us taking advantage of any of that.

If we expect that a tbzr user will ever need to use a text editor to change bazaar.conf, I see advantages in tbzr using the same library and file structure.  If in the distant future we get a GUI editor for the options in bazaar.conf, then we should also get an editor for a tbzr.conf file almost for free.  But a quick glance at config.py indicates it might not be that suitable for reuse.

As a final random point, although we are targetting win32 only, I am trying to keep it as platform agnostic as possible.  I see real benefit to non-windows users being able to run some of the core tests - if for no better reason that it becomes more practical to enlist non-windows using bzr wizards for help ;)  User-prefs will necessarily need to be involved in the tests.

So I'm personally leaning towards a file based structure, but more discussion is certainly invited.

> logging.py is actually a bit of a thorn in bzrlib's side. It is rather
> slow for
> an app that starts and stops a lot. Probably it would be quite good for
> tortoise's server, but bad for a client.

Excellent - that is the impression I got with bzrlib too.

> So that it runs when you run "bzr selftest"? 

I guess that is the long term intent, but for now, what I'd like is:
 
  tbzr.py selftest

(ie, *my* entry point script) to import bzrlib.tests, and tell it to find and run all tests in my test directory.  IOW, reimplement 'bzr selftest', but with a different place to locate the tests.  I suspect that might become easier if I reorg the tests as you suggest.
 
> | * Recursive status requests: currently we request the full recursive
...
> 
> See massive discussion on the mailing list.

I must have missed that one? ;)

Thanks!

Mark