-
Notifications
You must be signed in to change notification settings - Fork 187
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
Conversation
@juhalehtonen Thanks for this PR, appreciate it. Is is possible to sort those deps with status of |
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 |
Can you make the flag be 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 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 @ericmj : I had a cursory look at testing this and couldn't immediately see how I would test the ordering in I could probably do something like |
Here I was thinking I just missed ElixirConf ;)
You can try using |
Thanks for the 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 😄). |
There was a problem hiding this 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.
Co-authored-by: Eric Meadows-Jönsson <[email protected]>
Thank you @juhalehtonen! |
end) | ||
end | ||
|
||
defp maybe_sort_by(values, _), do: values |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks Wojtek!
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:
Thanks for taking the time! 🙂