-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
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.
Looks good to me!
lib/search/packages.ex
Outdated
else | ||
{:error, _} = err -> | ||
err |
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 adds some strictness but I don't think it's worth it and so I'd just remove it.
else | |
{:error, _} = err -> | |
err |
lib/search/packages.ex
Outdated
else | ||
err -> err |
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 clause doesn't do anything, why do we keep it?
lib/search/packages.ex
Outdated
else | ||
{:error, _} = err -> | ||
err |
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.
ditto:
else | |
{:error, _} = err -> | |
err |
releases = [ | ||
%Release{package_name: "lesser", version: Version.parse!("1.2.3")}, | ||
%Release{package_name: "greater", version: Version.parse!("2.1.3")}, | ||
%Release{package_name: "middle", version: Version.parse!("1.4.3")} | ||
] | ||
|
||
assert %Release{package_name: "greater"} = | ||
Release.latest(releases) |
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.
nitpick: this doesn't matter so I'm ok with keeping this as is but in practice we'd have the same package name but different version so I'd consider changing this and similar tests to be slightly more realistic:
releases = [ | |
%Release{package_name: "lesser", version: Version.parse!("1.2.3")}, | |
%Release{package_name: "greater", version: Version.parse!("2.1.3")}, | |
%Release{package_name: "middle", version: Version.parse!("1.4.3")} | |
] | |
assert %Release{package_name: "greater"} = | |
Release.latest(releases) | |
releases = [ | |
%Release{version: Version.parse!("1.2.3")}, | |
%Release{version: Version.parse!("2.1.3")}, | |
%Release{version: Version.parse!("1.4.3")} | |
] | |
assert Release.latest(releases).version == Version.parse!("2.1.3") |
Sorry for the late and big PR, I will make sure the next ones are more manageable 😅
There are several improvements to the architecture of the app: