Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addons: sorting algorithm for versions customizable on flyout #11069

Merged
merged 20 commits into from Mar 12, 2024

Conversation

humitos
Copy link
Member

@humitos humitos commented Jan 26, 2024

Allow users to choose one of the pre-defined algorithms:

  • Lexicographically
  • SemVer (Read the Docs)
  • CalVer
  • Custom pattern

The sorting algorithm is implemented in the backend. So, the list returned under addons.flyout.versions will be sorted acordingly the algorithm the user chose. There is no need to do anything extra in the front-end.

The algorithm follows the next general rule: "all the versions that don't match the pattern defined, are considered invalid and sorted lexicographically between them and added to the end of the list". That means that valid versions will appear always first in the list and sorted together with other valid versions.

There is one key feature here and is that the user can define any pattern supported by bumpver, which is the module we use behind the scenes to perform the sorting. See https://github.com/mbarkhau/bumpver#pattern-examples

On the other hand, SemVer (Read the Docs) is implemented used the exact same code we were using for the old flyout implementation. This is mainly to keep backward compatibility, but its usage is not recommended since it's pretty hard to explain to users and hide non-clear/weird behavior.


Screenshots

Screenshot_2024-01-26_14-33-51

Screenshot_2024-01-26_14-34-15

ToDo

  • write simple test cases for each algorithm
  • write test cases for project's form

Closes readthedocs/addons#222
Closes #7732

Allow users to choose one of the pre-defined algorithms:

- Lexicographically
- SemVer (Read the Docs)
- CalVer
- Custom pattern

The sorting algorithm is implemented in the backend. So, the list returned under
`addons.flyout.versions` will be sorted acordingly the algorithm the user chose.
There is no need to do anything extra in the front-end.

The algorithm follows the next general rule: _"all the versions that don't match the
pattern defined, are considered invalid and sorted lexicographically between
them and added to the end of the list"_. That means that valid versions will
appear always first in the list and sorted together with other _valid versions_.

There is one _key feature_ here and is that the user can define any pattern
supported by `bumpver`, which is the module we use behind the scenes to perform
the sorting. See https://github.com/mbarkhau/bumpver#pattern-examples

On the other hand, `SemVer (Read the Docs)` is implemented used the exact same
code we were using for the old flyout implementation. This is mainly to keep
backward compatibility, but its usage is not recommended since it's pretty hard
to explain to users and hide non-clear/weird behavior.

Closes readthedocs/addons#222

This comment was marked as outdated.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice pattern. I think we'll want to break this out into its own heading in the Addons settings to make it a bit nicer to configure. I know there's other conversation around that, but seems more important as we add options here.

readthedocs/projects/constants.py Outdated Show resolved Hide resolved
readthedocs/projects/models.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Jan 29, 2024

I'm not sure where my comment went, so I'm writing it again 🤷🏼


I think we'll want to break this out into its own heading in the Addons settings to make it a bit nicer to configure. I know there's other conversation around that, but seems more important as we add options here.

I suggested splitting this page into one section per addon in #11031, but I used a CrispyForm pattern we want to avoid. @agjohnson said he will have more time in the following days to take a look at that and communicate what's the pattern we want to follow there. Once that's defined, I'm happy to jump into the work to implement this pattern on this page 👍🏼

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • We should use the verbose name of the version to sort, not the slug.
  • Shouldn't we still list latest and stable first? Those are special versions, basically an alias for the two mayor versions.
  • Talking about latest and stable, shouldn't we use these same algorithms to get the stable version?

readthedocs/projects/version_handling.py Show resolved Hide resolved
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great feature addition, I know we have had this requested in the past.

I think this form looks okay for now, but would keep this UI mind for a later upgrade too. It is difficult and somewhat confusing trying to explain these options just using the option text description. One small upgrade might be to expose the underlying pattern for each as a read only field (similar to the project redirect UI), or even exposing a description and/or example for each option. Both of these would be JS/Knockout additions, and out of scope for a first pass though.

But for that, I wouldn't try to explain the options using the option text description either.

readthedocs/projects/constants.py Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Jan 30, 2024

@stsewd

Shouldn't we still list latest and stable first? Those are special versions, basically an alias for the two mayor versions.

This makes sense to me. I will give it a try. This may also be configurable, like "Show latest and stable at the beginning (default True)" and users can disable it if they don't like that approach.

Talking about latest and stable, shouldn't we use these same algorithms to get the stable version?

I'd say yes, but that's a big refactor 😄

@agjohnson
Copy link
Contributor

Shouldn't we still list latest and stable first?

I'd agree that latest/stable at the top of the listing should be preserved though. It won't always look out of place, but depending on the branch/tag naming the ordering could be a bit janky, ie:

  • dev
  • foo
  • latest
  • something
  • something
  • stable

This could be a case for trying to find a different place in the flyout UI/UX to describe latest and stable too.


Just in case there is some overlap here, I ran into this same issue on the version listing dashboard view. It would be great if the version listing filter had a default sort option that matched the documentation listing order, and showed latest/stable up front.

Either reusing the code in this PR, or eventually working this into the version filter and reusing that in the API could be options. Just noting for now though.

@humitos
Copy link
Member Author

humitos commented Feb 5, 2024

This could be a case for trying to find a different place in the flyout UI/UX to describe latest and stable too.

This is definitely something that will be pretty clear to users. I would consider this when redesigning the flyout in the next iteration. In the meantime, I think we can implement the simple behavior proposed by Santos that keeps latest and stable at the beginning with an option to opt-out.

Should it be stable latest ... or latest stable ...? What is the version we want the users to click on most of the times? cc @agjohnson @stsewd

@stsewd
Copy link
Member

stsewd commented Feb 5, 2024

I think it makes sense to list latest first and then stable, that's how they will be sorted if we use their aliases.

@humitos
Copy link
Member Author

humitos commented Mar 4, 2024

I updated this PR to bring latest changes and made strings translatable. I also implemented latest and stable at the beginning of the list. The only missing thing here is to write some tests as I put in the description. Once that's done, we can move forward with the review and merge of it.

I also opened some issues to discuss and track the suggestions done in this PR:

@humitos
Copy link
Member Author

humitos commented Mar 4, 2024

CircleCI checks fails because a new requirements is needed, but can't be installed there until the PR gets merged 🙃

ModuleNotFoundError: No module named 'bumpver'

It's fine to move forward, tho 👍🏼

@humitos humitos marked this pull request as ready for review March 4, 2024 15:33
@humitos humitos requested a review from a team as a code owner March 4, 2024 15:33
Comment on lines 75 to 78
"v1.0",
"1.1",
"1.1.0",
"2.5.3",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to invert this to show the newest version first in the flyout?

Now it will display:

v1.0  1.1  1.1.0   2.5.3

but maybe what we want is:

2.5.3  1.1.0  1.1  v1.0

This is how the old flyout sorts the versions (the newest first). Example: https://docs.godotengine.org/en/stable/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinging @astrojuanlu since you are one of the users wanting this feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think descending order makes more sense

Copy link
Member Author

@humitos humitos Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the result for this case using Python Packaging sorting will be:

2.5.3	1.1	1.1.0	v1.0

Then, the final result with Read the Docs special versions and invalid ones will be:

latest	stable	2.5.3	1.1	1.1.0	v1.0	another	invalid

The pattern is:

<latest if active> <stable if active> <versions matching the pattern descending> <invalid versions alphabetically ascending>

It makes more sense to show `latest  stable  <newest>` than `<oldest>` first.
#11069 (comment)
…mitos/addons/flyout-versions-sorting-algorithm
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good. Surprisingly complex for something I don't expect to be used too much by users. Definitely shows that the Addons config page is going to keep getting more complex, and we'll likely need to think more about how to display this stuff nicely.

readthedocs/projects/version_handling.py Show resolved Hide resolved
readthedocs/proxito/views/hosting.py Show resolved Hide resolved
@humitos humitos merged commit e4b9e12 into main Mar 12, 2024
5 of 6 checks passed
@humitos humitos deleted the humitos/addons/flyout-versions-sorting-algorithm branch March 12, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flyout: sort by version number, not lexicographically Allow to modify versions listed in the selector
5 participants