Code review comment for lp:~rharding/charmworld/bundle-metadata2

Revision history for this message
Richard Harding (rharding) wrote :

> Lines 44 and 85 of the diff: The name "data" is somewhat
> information-free, maybe bundle_data would be more informative.

Updated. It's actually the services bit from the bundle so went with bundle_charm_info.

> Line 45 of the diff: The docstring summery might be better rendered as
> "Construct a query that specifies a charm from data about that charm."

Updated

> Line 47 of the diff: The bulleted lists in the docstring have
> inconsistent indentation.

Same

> Lines 64 and 77 of the diff: Please capitalize the sentence in the
> comment.

Thanks

> Line 71 of the diff: I would be paranoid and pass "/trunk" to the
> .endswith() call so a branch name like "who-is-that-in-the-trunk" won't
> be matched.

Makes sense.

> Line 196 of the diff: It would be slightly better to use .assertIsNone().

Updated

> Line 357 of the diff: There is an extra space after the colon. I think
> lint will pick that up, so you should run lint to see if there is more.

Thanks, also had the extra line setting the maxdiff to none that lint caught and removed.

> Line 205 of the diff: The TestBundleLoadingCharms test case could use a
> test that shows what a request with too-little information to find a
> charm will produce.

Thanks! This misbehaved and the test caught that and got it adjusted.

« Back to merge proposal