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

> There are a few small things that should be addressed, but otherwise
> this looks good.
>
> It would be nice if the comment that includes "Bundle IDs can consist of
> up to 4 parts." spelled out explicitly what those four parts are.

Updated the comment with the parts vs the sample urls. I agree that it's clearer
that way.

> Line 96 of the diff: We should capitalize "id" unless we're also talking
> about the "ego" and "super-ego". I also feel that we should capitalize
> "URL" but that may be more of a personal preference, so I'll leave that
> one up to you.

Updated, not a fan of caps URL, but it'll be more consistent.

> Line 98 of the diff: A "split point" is referenced, but I don't see one.
> There is an identical comment earlier in the file; perhaps this is a
> copy/paste bug.

Agreed, updated comment to note the position change vs the 'split point'.

> The try/except around lines 110 through 119 of the diff seems a little
> big and the except is for two unrelated exceptions. I think it would be
> better to have less code in the try and handle each exception
> independently.

Agree in a sense. I moved the try/except around the one line of code that's attempting to find a version number. Both exceptions come out of that line of code though, so left the two exceptions to be caught there. Added a comment on why each was included to help clarify that one can come from the int() call and the other from the [index] use.

> Lines 130 and 162 of the diff: s/id/ID/
>
> make_bundle_file could use a docstring.

Updated! Thanks, saw a lot of methods sans docstrings and trying to find balance. I'm usually a docstring all the things view myself.

> Since test_extracting_bundle_id_with_trailing contains three independent
> setup/assert blocks it would be better to have it broken into three
> separate tests (each with their own customized version of the comment at
> the top of the test now).

Updated.