#693 Sorting the Release tags by name to display latest first #617
Merged by pingou. Opened by cverna.
cverna/pagure pagure_617  into  master

Download 693.patch
no initial comment

This is my take on issue #617.

Looks here that it only reverse the order compare to the actual code, 1.1.2 is still shown as higher than 1.1.11

The easiest way might be to find a way to split the tags into a list of (when possible) tuple of integers. Since tuple can be sorted easily, if we have tuples of int we should be able to sort them fine.

We should be careful though about tags such as foo-1.1.2-beta or 1.1-3, in other words it can easily gets messy :)

Looks here that it only reverse the order compare to the actual code, 1.1.2 is still shown as higher than 1.1.11

Oops yeah did not include this in my test case : )

                  The easiest way might be to find a way to split the tags into a list of (when possible) tuple of integers. Since tuple can be sorted easily, if we have tuples of int we should be able to sort them fine.

We should be careful though about tags such as foo-1.1.2-beta or 1.1-3, in other words it can easily gets messy :)

This sounds like playing with regexp to find the integers in the tag name

This sounds like playing with regexp to find the integers in the tag name

Could work, until someone comes up with 1.1.2-a and 1.1.2-b :-)

Note that I'm not trying to discourage you, we should try to go for something simple that works in most cases, but we should also be aware that the human mind is brilliant with creativity when it comes to versioning a project :)

Yeah no problem :) I ll play around with tag names and try to find something that works in most cases.

pingou, cverna,

would using distutils.version StrictVersion here give us the order we want?

i tried it out here, and it seemed to work for me, although didnt test all the weird versioning options we might encounter :)

https://pagure.io/fork/ryanlerch/pagure/917ca8053d94dc4e900ef4ac2e7b848a7e4a2542

https://pagure.io/fork/ryanlerch/pagure/commits/pagure_617

There is also an issue here with sorting on tag.name, because pagure.lib.git.get_git_tags_objects returns a commit object rather than a tag object if the tag is not an annotated tag.

git tag 0.1

vs

git tag -a 0.1 -m"version 0.1"

Currently (without this sorting), this also shows in the master releases page, as the tag doenst show a name if the tag is unannotated.

ryanlerch, pingou,

I have quickly tested the distutils.version StrictVersion and doesn't seems to handle the versions which are mixing letters and digits.

I have been playing a bit and I came up with the following :

https://pagure.io/fork/cverna/pagure/a890376c9d0198e006455aa862735fe7bf18b44f

https://pagure.io/fork/cverna/pagure/commits/pagure_617

I use the regex to split the tag name with any non-alphanumeric character, then for each element if the string is a digit I cast it to an integer and store it in a tuple.

I didn't know .isdigit(), this is neat!

I'm seeing a C coding style here, no?

I would have instantiate sorting_tuple directly in the loop instead of declaring it outside and resetting it at the end of each iteration.

nitpicking: missing a space after the comma

nitpicking: a space too much before the [

Couple of coding style comments but this is looking quite nice. Do we want to add some documentation to the code to describe what's going on?

I'll take your comments in. I can add a quick comment before the code to describe the logic.

I think it would be nice to have some unit test too, I haven't look at this part yet.

I think it would be nice to have some unit test too, I haven't look at this part yet.

This is always true, always :D

What do you think, about creating a function like 'sort_git_tags' in the lib/repo.py and then just make a call to this function in the ui/repo.py.

This would make it easier to document the code, via the function description, but also easier to unit test the function.

This also has the advantage of keeping logic out of ui/repo.py

Sounds like a good idea to me :)

Either that or adjusting pagure.lib.git.get_git_tags_objects, also an option but we'd have to be a little more careful and check if this method is used elsewhere.

I have moved the sorting algorithm to pagure.lib.git.get_git_tags_objects, as this method is only used once, to display the tags in the release page.

I have also created a unit test for pagure.lib.git.get_git_tags_objects.

The sorting works in most of the case, it just starts not to be to accurate when you mix integer and strings, ie 0.1.1 , 0.1.1-alpha , 0.1.1-beta will be sorted 0.1.1-beta, 0.1.1-alpha, 0.1.1. I am not quite sure how to solve this case.

Anyway if you want to have a look at my last commit :

https://pagure.io/fork/cverna/pagure/c99c18f04ab5fea283c675cc00500d1724c000b3

Adding a line w/ spaces here :)

Missing a space after the comma

The code looks good, the tests are looking great! Kudos!

I am trying to follow the pep8 standards for code style. Fedora provides a package python-pep8 which itself has a small utility called pep8 that you can invoke on your code and will help you to make it pep8 compliant.
While there are cases where readability is more important than coding style, it may also help you keeping the coding style of the project.
Would you be interested to adjust you PR for these? If not, just let me know and I'll merge the PR directly and we can deal with style at another time.

Aren't we in 2016? :)

I have pushed my latest changes

Looks all good to me!

Thanks to tackling it :)

Metadata