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

On 7/21/2011 3:06 PM, Vincent Ladeuil wrote:
> Review: Needs Fixing
> On behalf of UDD users: Thank You !
> 
> 27	+ c = the_branch.get_config()
> 28	+ verbosity = c.get_user_option('bzr.plugins.launchpad.packaging_verbosity')
> 
> You're a bit out of fashion here ;)
> 
> You can start using the the stck-based config here.
> 
> This can take various forms:
> 
> - the consensus about the option names would be more
>    'launchpad.packaging_verbosity' which is still long and may be
>    slightly unclear, how about 'lp.package_freshness' ?

Except, this is about package_branch_freshness, and I don't have good
ideas for what the values should be. "package_branch_freshness_check"
could work, but that is much more verbose.

freshness_verbosity
freshness_check
up_to_date_check

And the _check form doesn't really lend itself to "short" vs "minimal",
just to on/off.

I'm ambivalent about "lp." vs "launchpad.". I like the shortness, but it
does seem best to have unique namespace, and someone might create a
plugin named 'lp'. Then again, launchpad already provides lp:...


I'll go with "launchpad.packaging_verbosity" for now. And I'll update
the docs for this.

> 
> - use the stack-based implementation (even if you target 2.4)
> 
>     c = config.BranchStack(the_branch)
>     verbosity = c.get('bzr.plugins.launchpad.packaging_verbosity')
> 
>   Declare your option:
> 
>      option_registry.register('xxx, Option('xxx', default='off'),
>                               help='xxx is so nice, use it.')

I'm actually thinking to target 2.3 in the middle-term, because that is
where the developers using package branches are going to be at right
now. We may decide not to do it, but I'd like to at least plan for
backporting this for now. The time it takes to clean up now is the same
as it will take in the future.

>   
> 113	+def report_freshness(the_branch, verbosity, latest_pub):
> 
> This function seem to mix 3 different things:
> - get the LatestPublication data
> - time them
> - report to the user
> 
> I'm not sure we need to time them (mutter does that already no ?)

Mutter reports the time-since-started, and with a small amount of math
you can work out the delta. But, you still need to mutter *something*,
and it is easier to just mutter the info I wanted.

Long term, we can remove them. Short term, I think it is important to be
monitoring how much overhead adding this code is costing us, vs the
benefit to the users.

> 
> I think the code will be clearer and easier to test if this was
> split.
> 

Ultimately, there will always be a 'do what I want' function, and the
size of the 3 steps is pretty small. I did refactor it, but it didn't
change the number of tests I needed to do for the reporting function. It
made them a little bit smaller, I guess.

> 234	+class TestReportFreshness(tests.TestCaseWithMemoryTransport):
> 
> This can then be split/parametrized so the display tests just
> don't need a branch at all.
> 
> 252	+ orig_log_len = len(self.get_log())
> 
> In many discussions about self.get_log() we mentioned that only a
> few tests were requiring get_log() and I seem to remember that
> everybody agreed on that.
> 
> Here, you have to filter almost everything, istm that split the
> display out will make the filtering uneeded.

I did go this way, but it meant losing the ability to distinguish
'trace.note' from 'trace.warning'. I still want the data to go to the
log file (I certainly don't want it to go to stdout).

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

iEYEARECAAYFAk4pdx8ACgkQJdeBCYSNAANS/ACgrcFS07HjssqSHH3xRXnk8BSk
dBkAmgPM6W9w4paYwGgALWo/Quq2dHzT
=qc81
-----END PGP SIGNATURE-----