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

Revision history for this message
Benji York (benji) wrote :

The branch looks good. I saw a few little things that deserve your
attention.

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

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."

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

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

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.

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

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.

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.

review: Approve

« Back to merge proposal