Merge lp:~philip-peitsch/quickly/multiple-distros into lp:quickly

Design tackles two separate parts:

- Abstraction of version handling to make parsing easier and more consistent
- Finding the LSB distro information required

A --include-distro (yes/no) has been added to release & share. This updates the project's stored value upon calling. If not set, it is assumed false, and this setting is stored.

The behaviour for --include-distro=no is identical to previous definition. With --include-distro=yes, the string "-distroname1" is always appended to the current release or shared value. The number 1 at the end of the distro-name is never incremented (I didn't see a point to doing this at this stage).

Generated file names are as per the bug report. Unit tests have been added to verify the version recognition is behaving correctly.

As every, let me know what I can do to improve this patch :)

So, I looked at your change and I really like your work there about factoring the versioning scheme :)
Also the testsuite is very great and that motive me to expand it (it just should be in <template_directory>/test/, but that's a detail ;)

Now, about the multi distro thing, I think there is a little misconception, let me explain how it works.

In ubuntu, for instance, let's say you release 0.2-0ubuntu1 in karmic. If you don't change it, 0.2-0ubuntu1 will be rebuild on lucid (and during the dist-upgrade, it will fetch and download it). There is no need for ~<distrib> or other stuff like that which should be used at last resort (for corner case that one developer won't encounter on his/her own, normally).
Now, if a new version is released, we can as well backport to previous version, like 0.2.1-0ubuntu1 for instance.

And I'm sure you will tell me now that "oh, but it's failing in my ppa when I try to reupload the same version, just changing the distro in debian/changelog". Yes, but…

That's because uploading most time the same version is wrong wrong wrong (wrong? ;)). You should just ask for a rebuild against another version of the distribution.

- "Oh sweet, how can I do that?"
- "No pb son, just head to copy, select 'This' as destination ppa and ask for a rebuild against another release".
- "That was quite hidden"
- "Right"
- "So, what about providing a command to do that for the user?"
- "That's an excellent idea, but there is nothing in the API at the moment to due that"
- "So, for Quickly 0.6?"
- "yeah, let's say that"

So, what do you think about that plan? With that, we don't clutter the versionning and it's working better.

But I'm still very very interested in the other part of the patch which refactors and makes a testsuite from the versionning scheme and autobumping. I would just rename "parse_version" to "bump_version" or "parse_and_bump_version" to show that we are returning a new value.

Tell me what do you think about that plan and resubmit this part?

PS: sorry, I'm a little bit ill now and that's why some part are just on the fun side ;)

Ahh! That is exactly the type of explanation I was unable to really find or understand when I did this change :). Thanks very much for laying that out for me... makes a lot more sense now.

So, what I'll do with this patch is eliminate all traces of "include distro" details. Do you want me to remove the handling of that within the parse_version section? I think it might actually be a good idea to leave it in at the moment, because then the user can manually set the distro as a version if required (currently it will refuse to allow it).

Regarding the renaming, I see bumping the version and parsing to be to separate operations because each can potentially get more complex as more styles are supported. Keeping them separate makes unit-testing easier.

With the test directory, are you saying that it should be in /data/templates/ubuntu-application/test rather than /test?

I'll have to think about adding in support for multiple distros some other way then :). I'm hitting the issue where I'm developing in lucid, but want to release in karmic... and manually copying the package all the time is somewhat unattractive :). I'll keep thinking on a better way to do that (it could be as you say, interfacing with the launchpad API).

P.S., I think I quite like your ill patch reviews lol... that was a very entertaining summary to read :D. Thnx!

Ok. I've rolled out all the include distro stuff. I'll await your guidance on fixes to the test directory structure and what improvements / changes you want to parse_version.

Hope you get better :)

Thanks, I'm getting a little better today. I would be in shape for working next Monday, hopefully for my employer :)

I agree with parsing/replacing thing. I just misunderstood your function the first time as it returned a value. It's better with a clear mind now ;)

I'm quite not comfortable enclosing the user in a shema like -distro and we should free it a little bit… After some reflexion, I think we should:
On quickly release <version provided>, don't check anything (something in release.py like:
if not proposed_version:
    try:
        release_version = packaging.updateversion(proposed_version)
    except (packaging.invalid_versionning_scheme,
        packaging.invalid_version_in_setup), error_message:
        print(error_message)
        sys.exit(1)
else:
    release_version = proposed_version
(or maybe, we can do it in update_version directly, I have no strong opinion about that…

Oh, but it's already present in updateversion() \o/ (see the 5 first lines?). Heh, it seems I'm ok with myself, good! (this is not a joke, but so weird that I don't want to remove it ;)).

So, quickly release foobarversionning will work :)
Going to quickly share, looking at the code this time, I still agree with how it worked previously: it's adding -public1,2,3 whatever the version was in setup.py (either automatically or manually previously added). But it's after the parsing in your version now. So, I would be in favor in making the check in the case we don't share only. The bad part is that now your parse() function should be rewriten as it was used to separate the share version (-publicX) to the core one, maybe 2 functions is needed: one for seperate that and one for checking the "core version"?.
With that, quickly share will whatever the version is, either:
- add -public1 if the previous version wasn't shared
- bump the -public(X+1) part.

So, the "right version checking" will not be done in sharing, only in the "else" part.
The last case is someone tweeting directly the value in setup.py. I would say we should no more support that and change the error message like:
Release version specified in setup.py is not a valid version scheme like 'year.month(.z)' (needed for autobumping version by Quickly). If you want to specify yourself your version, you should use: quickly release %s" % version_string

With that, I think we address all cases, this needs some rewrite for your function and splitting and I can do it if you don't want to, but I think this is the right way to go. What's your thoughts?

Considering the test, I was thinking about /data/templates/ubuntu-application/test/ (each template having their test in their namespace seems logical, isn't it? I have no strong opinion about that to be honest :))

If I'm not clear do not hesitate to shout as well. Thanks for your work again, it's more than appreciated :)

I think this MR doesn't apply anymore, cleaning the list :)