-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Show dependencies for casks #17940
Show dependencies for casks #17940
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
be46dc4
to
1548daa
Compare
This comment was marked as resolved.
This comment was marked as resolved.
9e10925
to
7cf5c2c
Compare
This looks really good. I see that for formulae we also include a ✅ or ❌ depending on whether the dependency has already been installed. Do you think it's worth adding something like that for casks too?
As is this PR is already a nice improvement. I just think it would be a nice-to-have thing. Not a blocker though. |
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.
Thanks for your work here!
Some minor, optional, suggestions.
Also agree with the suggestion at #17940 (comment).
Co-authored-by: Carlo Cabrera <[email protected]>
I agree definitely agree that it would be great to have, but maybe it would be better to do that as a follow-up? My thinking is like this, it seems the best way to achieve the goal would be to import this function from def decorate_dependencies(dependencies)
deps_status = dependencies.map do |dep|
if dep.satisfied?([])
pretty_installed(dep_display_s(dep))
else
pretty_uninstalled(dep_display_s(dep))
end
end
deps_status.join(", ")
end But I don’t believe that that import is possible without causing a circular import. So that in turn would (probably) require this function to be moved to a shared location, where both places can import it. Then it grows into much bigger PR. Please correct me if I’m wrong. |
Yeah, I think that can be handled as a follow-up. |
Yes, I'm fine with handling this as a follow-up. Let's wait until the week begins to give other maintainers a chance to review this before we merge it. |
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, thanks @HaraldNordgren! I'm going to create a new tag later today so will hold off merging until after that.
Agreed that e.g. ✅ would be all of: nice to add, worth pulling into a shared location and worth doing in a follow-up PR.
Thanks for implementing this. Just want to note some missing features that would be nice to add later:
|
@MikeMcQuaid I looked into how to possibly show the installation status for a cask in the same way as it is done for formulas. And the logic around I think I am in over my head here. |
@HaraldNordgren If you can open a draft PR with whatever you've done so far: it'll be easiest to help there when we're talking about code. I believe in you and can help you more ❤️ |
Here is a WIP draft for the follow-up: #17982 |
When running info on a cask show its dependencies.
This mirrors how it already works for formulas.
Fixes #17600.