Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Add decorator to brew info $pkg dependency list to indicate Dependency#installed? #18922

Closed

Conversation

colindean
Copy link
Member

  • Replace simple join output with decorator method.
  • Define default decorator method.
  • The default decorator method shows green check if installed or red X if not.

Screen Shot 2013-04-02 at 9 10 19 PM

Using a decorator method here enables a user to override Homebrew#decorate_dependencies with whatever they desire. I like the green checkmark and red X, so I offer them for the default. An simpler, less unicode-y alternative would be to simply highlight the dependency package name in the given color.

My personal motivation here is to use the indication as a factor in determining if I want to install a package right now. If I look at a package's info and see that none of its dependencies are installed, and installing them may take a while, I may reconsider installing that package right now if I'm on battery or if I'm doing something that CPU or disk usage from compiling may negatively affect.

Future work would include a similar decorator for conflicting packages, displaying some kind of a warning character if a conflicting package is installed.

@SevenBits
Copy link

Sounds cool.

@cooljeanius
Copy link

An simpler, less unicode-y alternative would be to simply highlight the dependency package name in the given color.

This could be the automatically selected option when the HOMEBREW_NO_EMOJI environment variable is set.

@jacknagel
Copy link
Contributor

Please, let's avoid the proliferation of unicode output, optional or otherwise.

@adamv
Copy link
Contributor

adamv commented Apr 3, 2013

Create an external command that does this

@SevenBits
Copy link

Why, exactly? I'm pretty sure Terminal has Unicode support. I doubt it really matters anyway. Pretty much everything nowadays had Unicode support.

On Apr 3, 2013, at 12:57 AM, Jack Nagel [email protected] wrote:

Please, let's avoid the proliferation of unicode output, optional or otherwise.


Reply to this email directly or view it on GitHub.

@jacknagel
Copy link
Contributor

Why, exactly? I'm pretty sure Terminal has Unicode support. I doubt it really matters anyway. Pretty much everything nowadays had Unicode support.

On what versions of OS X? (This is rhetorical). Please look through the threads about the beer emoji. Stuff like this has disproportionate maintenance costs.

If this is really necessary, the deps should just be colored differently, but I'm ambivalent about even that. Someone should just make a brew installed-deps <formula> command.

@MikeMcQuaid
Copy link
Member

Given we have the environment variable and to hide unicode and show it otherwise on every brew install I'm actually up for using more unicode where it displays more information in a single character. Whether it does in this case or not I'm not sure.

@colindean
Copy link
Member Author

So, direction. I need it.

Suggestions:

  • As-is.
  • Display single character normally, but only highlight dependency when HOMEBREW_NO_EMOJI is set.
  • Only ever highlight dependency
  • Revert this, and create a separate command that checks dep install status

I'd like to do this:

  1. if HOMEBREW_NO_EMOJI highlight dependency, else display single unicode character normally
  2. Create deps command that outputs a list of dependencies, potentially in some kind of tree

I feel that № 1 is within scope of this PR. № 2 is a useful idea show dependencies and their status, with an eventuality to resolve the dependency tree, if such a command doesn't already exist.

Ultimately, I'll do whatever you all think is appropriate for the project.

@MikeMcQuaid
Copy link
Member

I think probably a separate (external) command for now perhaps makes more sense. Open to other ideas from other maintainers though.

@mxcl
Copy link
Contributor

mxcl commented Apr 6, 2013

I like it. And think it's very useful. Why you against it @jacknagel?

It would be necessary to detect non-tty and output with plainer text though I should think.

@jacknagel
Copy link
Contributor

I've stopped caring and checked out of this thread because nobody seems to care that Homebrew is going to look like a cartoon soon. Do whatever.

@cooljeanius
Copy link

nobody seems to care that Homebrew is going to look like a cartoon soon.

I care. That's why I brought up the HOMEBREW_NO_EMOJI environment variable in the first place.

@SevenBits
Copy link

Who cares, really? Programs evolve based on the wishes of their users. If you don't like where Homebrew is going, use another tool.

On Apr 6, 2013, at 1:57 PM, Jack Nagel [email protected] wrote:

I've stopped caring and checked out of this thread because nobody seems to care that Homebrew is going to look like a cartoon soon. Do whatever.


Reply to this email directly or view it on GitHub.

@adamv
Copy link
Contributor

adamv commented Apr 6, 2013

Please do this as an external command so we can test drive it before we decide whether or not to put it into core.

@adamv
Copy link
Contributor

adamv commented Apr 6, 2013

For the record, I'm -1. I'd like to see a core set of brew commands that do simple machine readable output, sort of like the porcelain levels of Git, and a set of command on top, or separate UI packages on top, that do the "angry clown salad" thing (think tig).

@MikeMcQuaid
Copy link
Member

It remains machine readable as-is; just relies on unicode characters. I think it's actually a good call in cases like this where a single unicode character communicates more than any non-unicode one does.

@SevenBits
Copy link

I agree.

On Apr 7, 2013, at 5:50 PM, Mike McQuaid [email protected] wrote:

It remains machine readable as-is; just relies on unicode characters. I think it's actually a good call in cases like this where a single unicode character communicates more than any non-unicode one does.


Reply to this email directly or view it on GitHub.

@colindean
Copy link
Member Author

I've taken action to respect HOMEBREW_NO_EMOJI when set.

@MikeMcQuaid: It remains machine readable as-is; just relies on unicode characters. I think it's actually a good call in cases like this where a single unicode character communicates more than any non-unicode one does.

This is my primary motivation for using a single unicode character. It seems that it would already be wise for a machine-reading program to set HOMEBREW_NO_EMOJI anyway.

Screen Shot 2013-04-07 at 6 27 03 PM

I will endeavor to submit in a separate pull request a new command deps that shows the dependencies alone.

@SevenBits
Copy link

I'm glad Unicode characters are being used. This was a good decision.

On Apr 7, 2013, at 6:38 PM, Colin Dean [email protected] wrote:

I've taken action to respect HOMEBREW_NO_EMOJI when set.

@MikeMcQuaid: It remains machine readable as-is; just relies on unicode characters. I think it's actually a good call in cases like this where a single unicode character communicates more than any non-unicode one does.

This is my primary motivation for using a single unicode character. It seems that it would already be wise for a machine-reading program to set HOMEBREW_NO_EMOJI anyway.

I will endeavor to submit in a separate pull request a new command deps that shows the dependencies alone.


Reply to this email directly or view it on GitHub.

@chdiza
Copy link
Contributor

chdiza commented Apr 9, 2013

Somehow with colors and dingbats the output of brew info is even harder to read than it already is.

It is already hard to read (prior to this proposal) because the list of deps is a comma-separated list and isn't columnized. The part under "Depends on:", whether colorized or dingbatized or not, should be like the output of brew list.

Also, it isn't true in this case that a single unicode character is conveying more than any non-unicode one. E.g., what's wrong with good ol' Y and N rather than the corresponding unicode dingbats?

@SevenBits
Copy link

@chdiza The "dingbats" stand out and give a bit of visual appeal.

@colindean
Copy link
Member Author

Any additional thoughts or direction on this?

@MikeMcQuaid
Copy link
Member

I'll have a look.

def decorate_dependencies dependencies
#necessary for 1.8.7 unicode handling since many installs are on 1.8.7
checkmark = Tty.green + ["2714".hex].pack("U*") + Tty.reset
xout = Tty.red + ["2718".hex].pack("U*") + Tty.reset
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does xout mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means cross out or just X. I didn't want to use a single letter variable that seemed more likely to evoke this very same question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe name them tick and cross?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that those are any more indicative of their use or content, since it's difficult to determine what character is assembled by the code there. I'd considered using yes and no, but then it'd be just as difficult to determine the character.

Maybe remove the coloring from there and rename xout to crossout. I think of ✝ when I think of cross. Then, have another variable set that adds the color to the characters. This makes it a little easier to read. Or, pulling those into a function would allow easier overriding for custom settings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cross_out sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update it here and on my other PR.

@MikeMcQuaid
Copy link
Member

This works for me but I'll wait for a +1 from another maintainer.

@colindean
Copy link
Member Author

Changed xout to cross_out per @MikeMcQuaid's suggestion.

@adamv
Copy link
Contributor

adamv commented Nov 5, 2013

I'm now -0 on this since I can turn it off with the EMOJI env var.

If @MikeMcQuaid is still OK with this, this needs to be rebased on master for review.

@colindean
Copy link
Member Author

Interestingly enough, I'm planning to work on that rebase tonight...

@MikeMcQuaid
Copy link
Member

Cool. Let's rebase and I'll get on it.

* replace simple join output with decorator method
* define default decorator method
* default decorator method shows green check if installed or red X if not
* only highlight dependency if HOMEBREW_NO_EMOJI is set
@colindean
Copy link
Member Author

Rebased. Interestingly enough, I found a bug caused by this comment's additions and fixed it, too.

@colindean
Copy link
Member Author

@MikeMcQuaid Any other feedback? I'd like to get this in before I have to do another big rebase ;-)

@MikeMcQuaid
Copy link
Member

@colindean Will review properly ASAP.

@MikeMcQuaid
Copy link
Member

Pushed. Thanks @colindean.

@colindean
Copy link
Member Author

Thanks, folks!

@jjbohn
Copy link

jjbohn commented Nov 11, 2013

I've started getting an error from this:
Error: undefined method '+' for nil:NilClass

Trace:

/opt/boxen/homebrew/Library/Homebrew/cmd/info.rb:141:in `decorate_dependencies'
/opt/boxen/homebrew/Library/Homebrew/cmd/info.rb:125:in `info_formula'
/opt/boxen/homebrew/Library/Homebrew/cmd/info.rb:123:in `map'
/opt/boxen/homebrew/Library/Homebrew/cmd/info.rb:123:in `info_formula'
/opt/boxen/homebrew/Library/Homebrew/cmd/info.rb:36:in `print_info'
/opt/boxen/homebrew/Library/Homebrew/cmd/info.rb:34:in `each'
/opt/boxen/homebrew/Library/Homebrew/cmd/info.rb:34:in `print_info'
/opt/boxen/homebrew/Library/Homebrew/cmd/info.rb:17:in `info'
/opt/boxen/homebrew/Library/brew.rb:91:in `send'
/opt/boxen/homebrew/Library/brew.rb:91

@cheister
Copy link

I think this PR #24188 should fix the undefined method error

MikeMcQuaid added a commit that referenced this pull request Nov 11, 2013
@colindean colindean deleted the feature/dependency-indicator branch November 11, 2013 22:29
yriveiro pushed a commit to yriveiro/homebrew that referenced this pull request Nov 26, 2013
* shows green tick if installed or red cross if not
* only highlight dependency if HOMEBREW_NO_EMOJI is set

Closes Homebrew#18922.

Signed-off-by: Mike McQuaid <[email protected]>
yriveiro pushed a commit to yriveiro/homebrew that referenced this pull request Nov 26, 2013
ehershey pushed a commit to ehershey/homebrew that referenced this pull request Apr 4, 2014
* shows green tick if installed or red cross if not
* only highlight dependency if HOMEBREW_NO_EMOJI is set

Closes Homebrew#18922.

Signed-off-by: Mike McQuaid <[email protected]>
ehershey pushed a commit to ehershey/homebrew that referenced this pull request Apr 4, 2014
@Homebrew Homebrew locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants