Code review comment for lp:~leonardr/lazr.restful/bump-version

Thanks for all the changes Leonard. IRC log below for the record (and sorry Bjorn if you read this - I normally put the quoted diff, but I had to rush to finish this one on time!)

17:02 < noodles775> leonardr: on line 45 of the MP diff, is this really the earliest version, or the earliest *active* version? (which
                    line 47 seems to imply?)
17:03 < leonardr> noodles775: inactive versions don't exist in the lazr.restful installation, only in peoples' memories, so the
                  concepts are equivalent
17:03 < leonardr> i can change the wording

17:04 < noodles775> leonardr: in which case, why can len(config.active_versions) == 0, but we still have an earliest_version? or is
                    that what you'll re-word?
17:04 < leonardr> noodles775: it's kind of confusing but i don't want to change the interface becuase that'll cause backward
                  incompatibility problems
17:04 < leonardr> there are two fields in the interface
17:04 < leonardr> active_versions and latest_version_uri_prefix
17:05 < leonardr> active_versions is a list, the other is a stirng
17:05 < leonardr> lazr.restful serves web services for all of those strings
17:05 -!- danilos [~danilo@canonical/launchpad/danilos] has quit [Ping timeout: 265 seconds]
17:05 < leonardr> so if active_versions is =['beta', '1.0'] and latest_version_uri_prefix is 'devel' there are three versions being
                  served
17:05 < leonardr> if active_versions is [] and latest_version_uri_prefix is 'beta' then there is one version
17:05 < leonardr> though i don't recommend doing that
17:06 < noodles775> So if active_versions is =['beta', '1.0'] and latest_version_uri_prefix is 'devel', why are we evaluating
                    earliest_version = 'beta', is that right?
17:06 < noodles775> leonardr: ^^
17:07 < leonardr> noodles775: i'm having trouble parsing that sentence, but i can tell you that in that case earliest_version is 'beta'
17:07 < leonardr> latest_version_uri_prefix can only be the earliest version if active_versions is empty
17:07 < noodles775> OK, as the code implies, I'm just confused by the names.
17:08 < leonardr> noodles775: i'm kind of leaning towards treating the last item in active_versions as latest_version_uri_prefix

17:08 < leonardr> but that would be a separate job
17:09 -!- salgado-lunch is now known as salgado
17:09 < noodles775> Right, that would make sense. (do you mind adding an XXX?)
17:09 < leonardr> sure
17:11 < noodles775> leonardr: just a style question, is the comment on the empty line 69 intentional?
17:11 < leonardr> yeah, it's like a para break. i don't know if we allow that though
17:12 < noodles775> Yeah, I think a blank line (without the comment) serves just as well, but if the style guide doesn't say either
                    way, whatever you prefer :)
17:12 < leonardr> style guide doesn't mention it
17:13 < leonardr> if i see a blank line i think the first comment was talking about the blank line and the second comment is different
17:13 < noodles775> leonardr: s/marker_interface/version_marker on line 72
17:13 < noodles775> sure
17:19 -!- danilos [~danilo@canonical/launchpad/danilos] has joined #launchpad-reviews
17:20 < leonardr> noodles775: i might as well make that latest_version_uri_prefix change because i haven't actually released a version
                  that uses it yet. if i get rid of it now there will be no backwards compatibility
17:20 < leonardr> problem
17:20 < noodles775> leonardr: great
17:20 < noodles775> Two other small things:
17:21 < noodles775> See if you think it's worth creating a helper for the repeats of line 145-146 - the helper could even print out
                    the ordered list items?

17:22 < noodles775> s/repeats/repititions sorry, bad day.
17:22 < noodles775> And s/an/a on line 180.
17:26 < leonardr> noodles775: i thought about putting that 145-146 stuff in the big helper method but decided it was easier to read if
                  i spelled it out every time
17:27 < noodles775> leonardr: OK.
17:28 < noodles775> leonardr: another typo, s/in/In on 138
17:30 < leonardr> noodles775, fixed everything so far
17:31 < noodles775> leonardr: Great, I think the text for versions 3.0 is incorrect? The method has changed to by_value?
17:31 < noodles775> leonardr: and with that, r=me.
17:31 < leonardr> yes, you're right
17:31 < leonardr> noodles775: if you can move on to
                  https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/multiversion/+merge/19110 i'll work on getting rid of
                  latest_version_uri_prefix
17:32 < noodles775> leonardr: sorry, I need to leave asap now, but I've got one more question for the this MP:
17:32 < leonardr> ok
17:32 < noodles775> Is the change in tales.py tested?
17:33 < noodles775> I can't see an example where we we've removed an op in a later version?
17:34 < leonardr> noodles775: i don't have details off the top of my head, but it is tested in that the wadl.txt test will fail if you
                  remove the code
17:34 < leonardr> noodles775: actually i can add a quick test that proves it works
17:35 < noodles775> OK, great. I'm out of here... 'night!