Closed juhalehtonen closed 3 months ago
@juhalehtonen Thanks for this PR, appreciate it. Is is possible to sort those deps with status of update-possible
first?
@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.
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"?
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 π
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? π
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.
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 π).
Thank you @juhalehtonen!
This PR aims to add a new flag as discussed in https://github.com/hexpm/hex/issues/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:
Thanks for taking the time! π