Code review comment for lp:~jml/bzr/lp-login-oauth-2
Revision history for this message
| Jonathan Lange (jml) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
On Thu, Dec 17, 2009 at 2:30 AM, John A Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
>>> === modified file 'bzrlib/
>>> - --- bzrlib/
>>> +++ bzrlib/
>>> @@ -111,7 +111,7 @@
>>> link_bug=None,
>>> dry_run=False):
>>> from bzrlib.
>>> - - LaunchpadService, BranchRegistrat
>>> BranchBugLinkRe
>>> + BranchRegistrat
>>> DryRunLaunchpad
>>> 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(
> from bzrlib.
> """)
>
> Is a "whole bunch of hard".
It doesn't work -- see http://
I tried a bunch of things based on suggestions from the #bzr channel.
Then, on poolie's recommendation, I gave up and moved everything to
scoped imports.
After I gave up, spiv made a suggestion:
<spiv> jml: if you do "from from bzrlib.
lp_registration as _lazy_lp_
that fix it?
I haven't tried it.
...
>>> Some basic comments by just looking at the code:
>>>
>>> +CACHE_DIRECTORY = os.path.
>>>
>>> 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/
>>
>> No tests, sadly.
>
> Couldn't you add a test that "lp_config_dir()" returns something that
> ends in ".cache/
>
> As for "~/.cache/
> 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.
>
I would have thought so too. launchpadlib was originally the simplest
thing that could possibly work, and is slowly improving as more people
write applications that use it.
> Possibly "bzr_launchpad" since that is roughly the plugin's name. In
> which case going to bzrlib.
> more reasonable location. (And it gives you an APPDATA location on
> windows without having to re-code all of that again.)
>
Done.
>>
>>> 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.
>
I'm not at all upset with the review, and am sorry for seeming so.
It's a reasonable request, but I think it's actually quite difficult
to do well, and that it's not necessary for landing this patch. I'll
happily file a bug about it once the patch lands.
...
>>
>> 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?)
>
We do. I made a note to do this, but forgot to actually do it. It's
now done, with tests.
I haven't been able to figure out how to write a test that checks to
see what happens if launchpadlib is not available at all.
> 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?
>
Yes, I think that's the only thing we can do.
>>
>>> 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.
>
I've added some smoke tests. I'm not satisfied with them, but I think
they're enough for landing for now. Once this lands, I'll file a bug
about having a good testing strategy for commands that depend on
launchpadlib.
jml