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

Provide --sort flag sorting outdated results by status #1017

Merged
merged 7 commits into from
Apr 10, 2024

Conversation

juhalehtonen
Copy link
Contributor

This PR aims to add a new flag as discussed in #1005 to sort results by their status (whether an update can be performed or not).

The PR doesn't aim to provide a generic "sort by column" functionality, as the default sort is already by package name, and sorting by the other remaining fields (current or available version numbers) doesn't seem very useful.

At the time of writing, this PR is rather exploratory in nature and is seeking general feedback on direction. Does a primitive solution like this suffice here, or are there important things I'm missing here?

Tests are currently also missing, as I wanted to verify the general idea first. In any case, the primitive case seems to work:

# With default sorting:

juha@local:~/projects/elixir/hourchestra$ mix hex.outdated
Dependency              Current   Latest    Status
ash                     2.19.5    2.19.12   Update possible
ash_phoenix             1.2.20    1.3.1     Update possible
benchee                 1.0.1     1.3.0     Update not possible
credo                   1.7.1     1.7.5     Update possible
gettext                 0.23.1    0.24.0    Update possible
gnuplot                 1.22.270  1.22.270  Up-to-date
jason                   1.4.1     1.4.1     Up-to-date
libgraph                0.16.0    0.16.0    Up-to-date
oban                    2.16.3    2.17.5    Update possible
phoenix                 1.7.9     1.7.11    Update possible
...

# With the new flag:

juha@local:~/projects/elixir/hourchestra$ mix hex.outdated --by-status
Dependency              Current   Latest    Status
gnuplot                 1.22.270  1.22.270  Up-to-date
jason                   1.4.1     1.4.1     Up-to-date
libgraph                0.16.0    0.16.0    Up-to-date
solverl                 1.0.16    1.0.16    Up-to-date
benchee                 1.0.1     1.3.0     Update not possible
phoenix_html            3.3.3     4.0.0     Update not possible
ash                     2.19.5    2.19.12   Update possible
ash_phoenix             1.2.20    1.3.1     Update possible
ash_postgres            1.3.56    1.5.14    Update possible
gettext                 0.23.1    0.24.0    Update possible
oban                    2.16.3    2.17.5    Update possible
...

Thanks for taking the time! 🙂

@kianmeng
Copy link
Contributor

kianmeng commented Mar 9, 2024

@juhalehtonen Thanks for this PR, appreciate it. Is is possible to sort those deps with status of update-possible first?

@juhalehtonen
Copy link
Contributor Author

@juhalehtonen Thanks for this PR, appreciate it. Is is possible to sort those deps with status of update-possible first?

Definitely! The exact ordering can be any way we'd want it to be. However, I do agree with the sentiment that the user will probably run mix hex.outdated primarily with the intent of seeing which dependencies are outdated (if any), so taking them to the top makes a lot of sense from the UX perspective.

@ericmj
Copy link
Member

ericmj commented Apr 9, 2024

Can you make the flag be --sort status instead so we can add more sortings in the future in a structured way?

Would it make more sense to sort in the reverse order since a large list of dependencies will likely hid the important information behind the terminal window "fold"?

@juhalehtonen
Copy link
Contributor Author

Thanks for sharing your thoughts @ericmj !

I agree that --sort status makes for a more future-proof API, so definitely a thumbs up for that. Similarly, I agree that the reversed order can make for a better user experience now that you point out the limited real-estate of a terminal window.

I'll look into making these changes (and associated tests) in this PR soon -- probably after ElixirConf, though 🙂

@juhalehtonen
Copy link
Contributor Author

I actually went ahead and pushed a first iteration of the changes out already. The PR now supports passing a --sort status flag instead, and the results are now (implicitly) sorted in the order of "Up-to-date, Update not possible, Update possible". Perhaps an explicit sorting by these specific statuses would be better, or WDYT?

@ericmj : I had a cursory look at testing this and couldn't immediately see how I would test the ordering in test/mix/tasks/hex.outdated_test.exs. Using assert_received probably doesn't cut it as there's no guarantee of message ordering that way.

I could probably do something like Process.info(self(), :messages) within the tests after all messages are received and then ensure the resulting substrings are found in the expected order, but I have a nagging feeling that there's a less clunky way to do this (or perhaps one that doesn't require asserting arriving message ordering at all). Would you have any ideas or pointers for testing this? Perhaps we already test something similar elsewhere, like the headers themselves being first in line? 🙂

@ericmj
Copy link
Member

ericmj commented Apr 9, 2024

I actually went ahead and pushed a first iteration of the changes out already.

Here I was thinking I just missed ElixirConf ;)

I had a cursory look at testing this and couldn't immediately see how I would test the ordering in test/mix/tasks/hex.outdated_test.exs.

You can try using flush() which will return all messages in the mailbox as an ordered list: https://github.com/hexpm/hex/blob/main/test/support/case.ex#L22.

@juhalehtonen
Copy link
Contributor Author

Thanks for the flush/0 tip! The output of that is exactly what I was thinking I'd get with my initial Process.info(self(), :messages) idea, but you saved me a bunch of time I would've put in eventually rewriting that exact helper 😉

I pushed a preliminary commit that implements a single test case -- just to see that the thing is actually working. If you think the general direction here seems reasonably correct, I can continue with this work later (perhaps this time really after I get back from ElixirConf 😄).

@juhalehtonen juhalehtonen changed the title Provide --by-status flag sorting outdated results by status Provide --sort flag sorting outdated results by status Apr 9, 2024
Copy link
Member

@ericmj ericmj left a comment

Choose a reason for hiding this comment

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

A few suggestions, otherwise looks great.

I pushed a preliminary commit that implements a single test case -- just to see that the thing is actually working. If you think the general direction here seems reasonably correct, I can continue with this work later (perhaps this time really after I get back from ElixirConf 😄).

I think that test is enough.

test/mix/tasks/hex.outdated_test.exs Outdated Show resolved Hide resolved
lib/mix/tasks/hex.outdated.ex Show resolved Hide resolved
lib/mix/tasks/hex.outdated.ex Outdated Show resolved Hide resolved
@ericmj ericmj merged commit df071f0 into hexpm:main Apr 10, 2024
12 checks passed
@ericmj
Copy link
Member

ericmj commented Apr 10, 2024

Thank you @juhalehtonen!

end)
end

defp maybe_sort_by(values, _), do: values
Copy link
Member

Choose a reason for hiding this comment

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

This should have been defp maybe_sort_by(values, nil), in other words, we shouldn't silently ignore incorrect sort-by value. I fixed it on main: f4e76b6.

Thank you for the PR, this is a really nice feature!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks Wojtek!

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.

4 participants