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

fix: sort listing by version number in natural order #1100

Conversation

kianmeng
Copy link

@kianmeng kianmeng commented Dec 3, 2021

Summary

This PR fixes the issue where the installed versions from asdf list or asdf list <plugin command are not sorted in natural order.

Other Information

Before:

$ asdf list elixir
   ...
  1.8.0-otp-20
  1.8.1
  1.8.1-otp-21
  1.8.1-otp-22
  1.8.2
  1.8.2-otp-21
  1.8.2-otp-22
  1.9
  1.9.1
  1.9.1-otp-22
  1.9.2-otp-22
  1.9.4
  1.9.4-otp-22
  master
  master-otp-24

After:

$ asdf list elixir
  ...
  1.12
  1.12.0
  1.12.1
  1.12.1-otp-24
  1.12.3-otp-23
  1.12.3-otp-24
  1.13.0-rc.0-otp-24
  1.13.0-rc.1-otp-24
  master
  master-otp-24

@kianmeng kianmeng requested a review from a team as a code owner December 3, 2021 19:55
@jthegedus
Copy link
Contributor

jthegedus commented Dec 5, 2021

Hi @kianmeng are you able to share resources on how portable ls -v is? We were previously using -1 elsewhere so not concerned with that. We would need to ensure -v works on all major shells and OSs we support.

Tests would also need to be updated, ideally with versions that exhibit this behaviour should a regression occur.

@kianmeng
Copy link
Author

kianmeng commented Dec 5, 2021

Hi @kianmeng are you able to share resources on how portable ls -v is? We were previously using -1 elsewhere so not concerned with that. We would need to ensure -v works on all major shells and OSs we support.

I will assume the supported OSes are GNU/Linux and macOS and supported Shells are Bash, Zsh, and Fish? Is there any minimum version requirement for these Shells?

Tests would also need to be updated, ideally with versions that exhibit this behaviour should a regression occur.

Noted.

@jthegedus
Copy link
Contributor

I will assume the supported OSes are GNU/Linux and macOS and supported Shells are Bash, Zsh, and Fish? Is there any minimum version requirement for these Shells?

This is mostly it. OSs: GNU/Linux, macOS. Shells: Bash, ZSH, Fish, Elvish.

Is there any minimum version requirement for these Shells?

We don't have official version targets (something we wish to capture), but as low as possible.

@@ -95,8 +95,7 @@ list_installed_versions() {
plugin_installs_path="$(asdf_data_dir)/installs/${plugin_name}"

if [ -d "$plugin_installs_path" ]; then
for install in "${plugin_installs_path}"/*/; do
[[ -e "$install" ]] || break
for install in $(ls -1v ${plugin_installs_path}); do
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to achieve this without ls? I've been considering banning ls from the codebase because it's caused a couple bugs in the past.

Copy link
Author

Choose a reason for hiding this comment

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

None I can think of or found. Perhaps we can't or shouldn't implement this feature since using ls is not universal applicable for all platforms.

Your thoughts? @jthegedus @Stratus3D

Copy link
Member

Choose a reason for hiding this comment

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

Would it not be possible to do this with sort?

Copy link
Member

Choose a reason for hiding this comment

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

I guess part of the problem here is that when we list all versions of a plugin, we call out to the plugin's list-all callback, which is responsible for returning versions in the correct order. Here we are dealing with a subset of those same versions, but we don't have a way of calling out to the plugin to sort them.

Copy link
Member

@Stratus3D Stratus3D Dec 22, 2021

Choose a reason for hiding this comment

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

I suppose we could add another plugin callback that would receive a list of versions and would be expected to sort them, but I'm not sure it would be worth the hassle (we'd have to implement it in every plugin, or default to sorting versions as semantic versions.

@Stratus3D Stratus3D mentioned this pull request Dec 28, 2021
@Stratus3D
Copy link
Member

Thanks for your PR @kianmeng . I'm sorry I did not give you feedback sooner. We decided to move forward with banning ls in the codebase as its output is intended for human use and not machine. There are also some portability issues as not every platform supports every flag. Instead we recommend using find to achieve the same thing.

If you can achieve this with find we may consider an alternative approach. Closing this PR for now.

@Stratus3D Stratus3D closed this Jun 9, 2022
@kianmeng
Copy link
Author

kianmeng commented Jun 9, 2022

@Stratus3D Noted, and thanks for the explanation.

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.

None yet

3 participants