-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 >> === modified file 'bzrlib/plugins/launchpad/__init__.py' >> - --- bzrlib/plugins/launchpad/__init__.py 2009-12-07 15:47:05 +0000 >> +++ bzrlib/plugins/launchpad/__init__.py 2009-12-09 21:04:18 +0000 >> @@ -111,7 +111,7 @@ >> link_bug=None, >> dry_run=False): >> from bzrlib.plugins.launchpad.lp_registration import ( >> - - LaunchpadService, BranchRegistrationRequest, >> BranchBugLinkRequest, >> + BranchRegistrationRequest, BranchBugLinkRequest, >> DryRunLaunchpadService) >> if public_url is None: >> try: >> >> ^- This change made me wonder, but it looks like we are importing it at >> the module level. But if we are, then why aren't we importing the rest. >> >> Also, it seems like it might be nicer to lazily import lp_registration. >> I wouldn't expect a huge improvement to bzr startup time, but maybe >> something? >> >> Anyway, nothing you have to tweak, just a surprise on my part. >> > > I tried to change this to use lazy_import. It turns out to be a whole bunch of hard. > For this specific part, I don't see how: from bzrlib.lazy_import import lazy_import lazy_import(globals(), """ from bzrlib.plugins.launchpad import lp_registration """) Is a "whole bunch of hard". However, if you are using these objects at import time, just doing that at the top of the module seems reasonable. (If you import LaunchpadService, you have done all the work to import BranchRegistrationRequest, etc, you just haven't brought them into the local namespace.) ... > >> Some basic comments by just looking at the code: >> >> +CACHE_DIRECTORY = os.path.expanduser('~/.launchpadlib/cache') >> >> Doesn't seem quite right for Windows. This does generally have a value, >> but it isn't the place where you would put that sort of thing as a >> Windows user. >> > > I've tweaked this a bit so that it behaves like config_dir(). I've also changed the unix path to be ~/.cache/launchpadlib, which is slightly more standards compliant. > > No tests, sadly. Couldn't you add a test that "lp_config_dir()" returns something that ends in ".cache/launchpadlib" ? As for "~/.cache/launchpadlib" I would have thought that *launchpadlib* would have had a recommended cache location. If it doesn't, then we shouldn't be using the name "launchpadlib", since we are *using* the plugin, not authoring it. Possibly "bzr_launchpad" since that is roughly the plugin's name. In which case going to bzrlib.config.config_dir()+'/launchpad' would be a more reasonable location. (And it gives you an APPDATA location on windows without having to re-code all of that again.) > >> It also seems surprising that 'lp-login' doesn't actually use the same >> credentials path or storage that 'lp_api.login()' ends up using. >> > ... >> Which means that after running "bzr lp-login" doing "bzr lp-mirror" will >> (unless I'm misreading something) not actually work. >> > > I think you are misreading something. I can run lp-login and then lp-mirror. You seem to be upset with the review, and I'm sorry if I've put you on the defensive. I feel like having "bzr lp-login" share its information with "bzr lp-mirror" is a reasonable request. Having run "bzr lp-login" and reporting who I am, if I then run "bzr lp-mirror" it will ask me who I am. It isn't that it won't work at all, but it certainly surprising to give your credentials to a "lp-login" command, and then have to give it again to an actual "lp-do-something" command. I realize that they are doing separate things, and 'lp-login' has to do with how the 'lp:' directory service resolves. However, they *are* both provided by the same plugin, and one would think that the commands would know about each other. Even more so since one of them claims to be a "login" action. That said, you could respond with "yes that is odd, but it is outside the scope of this work [aka too hard] to get lp-login to interoperate with launchpadlib based commands. Perhaps I should file a bug?" > > Which is terrible. > > When I first wrote this branch, that was the only choice. Since then, I added features to the Launchpad API to get branches based on their URLs, so this logic is no longer needed. I've rewritten this code to use that API, which makes it much simpler and removes the need for the _service attribute. So since launchpadlib has been updated, do we need to add version checks/etc so that things 'break' in a clean manner. (If I have launchpadlib 1.really-old installed, would I get a reasonable error rather than something like AttributeError?) This, IMO, is one of the difficulties with 3rd-party libs. "I added features" and those features start getting used. Which is what progress is all about. But if we aren't going to provide a direct method for keeping in lock-step, then we need to provide a way to at least degrade gracefully. I guess if people install from packages then the packager ensures compatibility, and if they don't, we leave them to fend for themselves? > >> There also doesn't seem to be any tests that 'bzr lp-mirror' exists or >> even does anything. (Which has a small advantage that you don't have to >> worry about skipping those tests when launchpadlib isn't available.) >> > > I'm clinging to that small advantage, since you haven't actually made a recommendation here. > > jml > The recommendation was to have at least a smoke test that "the command exists or even does something". Which wasn't spelled out, but was certainly textually present. John =:-> -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (Cygwin) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkso+2IACgkQJdeBCYSNAAOTcwCdFwhlVYxDXXGlIIoW5MkAkxff IlEAoIsrWNEKrgp4YCkEqMm6/ZPFP6Gb =SY95 -----END PGP SIGNATURE-----