simple/<package_name> sort versions correctly

[cryvate]

It now sorts based on name, pkg_resource parsed version and reversed filename (keep extensions together). Fixes #2217.

Is there a way to extract the extension of the file cleanly? And the remaining metadata for e.g. wheels? Then we can sort on that instead of the 'hacky' reversed filename.

[cryvate]

I tested this with aws_kpl_deagg which has versions 1.0.1 to 1.0.10 inclusive in the server I got from following the development guide.

[cryvate]

I will add unit tests if this looks good, I am just pondering how to deal with the remainder of the filename properly (the extension etc.).

[di]

I don't think this needs to sort on file.name, as this is going to be the same for all files within a project.

Instead of sorting on the reversed filename as a secondary sort, you can use file.packagetype instead.

At this point, the key function should be short enough that you can just do it inline, e.g.:

    files = sorted(
        request.db.query(File)
        ...
        .all(),
        key=lambda f: (pkg_resources.parse_version(f.version), f.packagetype)
    )

[di]

@cryvate Apologies that this took so long to review! If you are still able to work on this, and get it merged, we'd be able to send you some PyPI stickers as part of Ernest's PyPI Sticker Challenge.

[cryvate]

@di I made the suggested change, and tried to update one of the tests. I cannot run the tests locally at the moment, so not sure it is actually testing what I've fixed. I'd love some stickers!

[di]

@cryvate Thanks! I think instead of modifying an existing test, it'd be better to add a new test here that would have failed before you made this change.

I'm thinking something like:

def test_file_sorting(self, db_request):
    project = ProjectFactory.create()
    releases = [
        ReleaseFactory.create(project=project)
        for version in ["1.0", "2.0", "10.0"]
    ]
    for release in releases:
        FileFactory.create(
            release=release,
            packagetype="sdist",
            filename="{}-{}.tar.gz".format(project.name, release.version),
        )
        FileFactory.create(
            release=release,
            packagetype="bdist_wheel",
            filename="{}-{}.whl".format(project.name, release.version),
        )
    files = simple.simple_detail(project, db_request)["files"]
    
    # TODO: make an assertion based on the order of `files`

I think the easiest way to make the assertion for that test would be to just look at the order of the filenames for each file.

Also, if you want to debug what's wrong with testing environment, feel free to ask questions here. It's a lot faster to be able to run tests locally!

[cryvate]

@di work on this is stalled until the 5th as I do not have internet at the moment, and the testing infrastructure does not seem very happy with that!

[di]

Hi @cryvate, hope you're back online! Are you able to run the tests locally yet?

[cryvate]

I did have a stab at it, but I couldn't get it to work. What's the best way to get feedback on what's going wrong, pypa IRC?

[di]

@cryvate Sure, or pypa-dev would maybe be more appropriate.

[brainwane]

@cryvate I wanted to check in -- if you are working on this issue and have questions, please feel free to ask them here, in #pypa-dev on Freenode, or on the pypa-dev mailing list. And then we'd love for you to add any troubleshooting tips we find together in a PR to improve https://warehouse.readthedocs.io/development/getting-started/#running-tests-and-linters .

[brainwane]

I've just pinged @cryvate via email.

[brainwane]

@cryvate letting you know about my comment #2217 (comment) . We're very happy to help you with this issue or with another issue if you're up for it, now or when you have time to help with Warehouse. Thank you!

[cryvate]

So there is a potential for a problem here, if f.packagetype is None problems occur (is that valid? I can make it happen in the tests but not sure it can happen in a real deployment)

[cryvate]

And similarly, if f.version is None? Looking at recent changes this happens relatively regularly.

[di]

File.version and File.packagetype will not be None here, and if they are, we've got bigger problems.

Also, I think I would prefer if we used packaging.version.parse here instead, from the packaging package.

[cryvate]

We should be good to go then

[brainwane]

@cryvate is this ready for re-review?

[cryvate]

@brainwane: yes

[lgh2]

It looks like some searches are still returning older versions of packages in certain circumstances. It looks like it might be something related to this comment.

This is a search run on pypi.io:

And this is the same search run on the branch:

[di]

@lgh2 This issue is about the order of links on the simple index for a project, e.g. https://pypi.org/simple/pip/

The search on pypi.org/pypi.io and the development environment is always going to differ, because the dev environment just has a snapshot (and, it's of TestPyPI).

[brainwane]

@cryvate Thank you for this fix and thank you for persisting through all the developer environment troubles you had!

Unfortunately I think @ewdurbin is now out of stickers :( (Ernest tell me if I'm wrong.)

@cryvate I really appreciate your work on this and would be happy to recommend other things around Warehouse that would benefit from your attention.