alexschimpf / fastapi-versionizer

FastAPI Versionizer
MIT License
79 stars 13 forks source link

Sorted routes is not natural sorted #22

Closed ndejong closed 11 months ago

ndejong commented 11 months ago

Subject of the issue

Route sorting is not naturally sorted causing the presented order of routes to not present in a manner as might be expected by humans.

Your environment

Steps to reproduce

Expected behavior

Actual behavior

ndejong commented 11 months ago

@ndejong proposed a PR to fix - https://github.com/alexschimpf/fastapi-versionizer/pull/23 @alexschimpf notes that the fix does not handle sorting of semantic versioning - https://github.com/alexschimpf/fastapi-versionizer/pull/23#issuecomment-1730342930

Rather than trying to guess how a user wants their sort to work, I wonder if we can inject a user-defined callable that then allows the user to establish their own sorting - this way we'd avoid needing to add another dependency and a user can sort in whatever way they want.

alexschimpf commented 11 months ago

Yeah, I think that makes sense. I've done something similar with my PR: https://github.com/alexschimpf/fastapi-versionizer/pull/18/files Added a version_sort_key parameter to be used in cases where versions need custom sorting. By default, versions won't get sorted correctly either because we're just using sorted by itself.

The sorting behavior of versions need not be the same as the sorting behavior of routes, so I guess it probably deserves its own sort key function.

I would imagine most folks probably expect natural sorting by default, so maybe it makes sense to do the following:

  1. Allow people to sort versions with a custom sort key function
  2. Allow people to sort routes with a custom sort key function
  3. Use natural sorting as the default behavior for both routes and versions
alexschimpf commented 11 months ago

@ndejong How do you feel about this solution? https://github.com/alexschimpf/fastapi-versionizer/pull/29

I think the most straightforward solution to this problem is using tuples for route keys instead of strings.

We can address a new feature to allow for a custom sorting function in a separate PR.

ndejong commented 11 months ago

Simple and effective and probably deals with many/most requirements.

The only side bar comment I'd make is that natsort also has a natsort_key function that in turn makes it possible to use built-in sorted() with the key parameter. I'm staring at that wondering if it's worth the effort to use natsort_key now so that adding custom sort is straight forward later. I guess we could reach for this if it makes sense later to create custom sort.

Your PR deals with most common situations, lock it in.

alexschimpf commented 11 months ago

Yeah, I'm curious how many folks even desire such customized sorting behavior. I think this will solve the vast majority's of people's needs. We can circle back on it if people seem to be wanting it.

alexschimpf commented 11 months ago

Versions 1.2.0 has been deployed to PyPI. Closing this issue.