-
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
Changed hex.outdated to show if a dependency can be updated #323
Conversation
It now takes all dependent packages into account and simply prints a Yes or a No for outdated packages. Fixes hexpm#322
Build fails for OTP 17.5 with errors that seem unrelated to my changes. Is that expected? |
Not expected, but it doesn't seem to be related to your changes :). |
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.
I left two minor comments. I would like one more review before merging.
@@ -1,5 +1,8 @@ | |||
## v0.14.2-dev | |||
|
|||
### Bug fixes | |||
* mix hex.outdated correctly tests if a package can be updated | |||
|
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.
You can leave it but in the future please don't change the changelog because it easily causes merge conflicts.
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.
Roger 👍
deps | ||
|> get_requirements(dep.app) | ||
|> Enum.map(fn [_, req_version] -> req_version end) | ||
|> List.insert_at(0, dep.requirement) |
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.
I would do [dep.requirement|requirements]
instead but you can leave as-is.
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.
I agree that this is a bit ugly. Will change.
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 an approve
Looking for one more review. /cc @wojtekmach |
Thank you! |
💗 |
It now takes all dependent packages into account and simply prints a
Yes
or aNo
for outdated packages.Fixes #322
The added test would show with a green "Requirement" before this update, even though it can't be updated.