Colorize things intelligently #155

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

mar-kolya commented Apr 24, 2014

This improves logic that decides weither to use colors in output:

  1. If --color or --no-colort params are provided act accordongly
  2. IF PINTO_NO_COLOR is defined act accordingly
  3. If connected to TTY use color, do not use color if not connected to TTY

This allows things like pinto list | grep Module work more naturally.

Colorize things intelligently
This improves logic that decides weither to use colors in output:

1) If --color or --no-colort params are provided act accordongly
2) IF PINTO_NO_COLOR is defined act accordingly
3) If connected to TTY use color, do not use color if not connected to TTY

This allows things like pinto list | grep Module work more naturally.

Is this so you can see the colors after piping ?

Owner

mar-kolya replied Apr 24, 2014

It is so I do not see colors after piping by default, basically

Owner

mar-kolya replied Apr 24, 2014

Most of the tools you can pipe through won't expect colors there - sort, grep, etc. This confuses user.

This patch removes this confusion, so user can still grep for his module without worrying about colors.

I've spent a considerable amount of time figuring out why my grep wasn't returning what I expected only to realize that colors were involved.

I think that 'not colorizing piped output' behaviour is 'much less surprising'.

Owner

mar-kolya replied Apr 24, 2014

Also, 'grep', for example has same behaviour:

cat file | grep 'foo' -- this will colorize things
cat file | grep 'foo' | less -- this will not colorize things

I implemented a bit differently, but I think this should do what you want.

Code is on the topic/colorizing branch. Care to take it for a spin before I release?

A summary of changes is in the Changes file.

Owner

mar-kolya replied Apr 25, 2014

Hmm, I've just pulled and do not see new branch there...

My bad -- I forgot to push. It is there now.

I got a little carried away and added a bunch of stuff to version the communication protocol (the protocol will need to be rewritten eventually). I should have started a separate branch, but I didn't :(

So if you want to see a diff of the colorizing code, this is probably a good one:

thaljef/Pinto@master...6d086d5

Owner

mar-kolya replied Apr 25, 2014

The diff looks good to me. I didn't try it out yet since it would require server upgrade...

Owner

mar-kolya replied Apr 25, 2014

I guess the only with I have is to somehow have this fix in current version. The fact that protocol has changed suggest that this feature would have to wait until 'major' release.

Owner

thaljef commented Apr 24, 2014

I think that 'not colorizing piped output' behaviour is 'much less surprising'.

Agreed. Not sure why I didn't do it that way to begin with. Perhaps because I often do this:

pinto list | -R less

I'm probably going to implement it differently, but I think we are on the same page.

FWIW, you can also use the -D and -P options to filter the list output

Owner

thaljef commented Apr 24, 2014

Not sure why I didn't do it that way to begin with.

Another reason might be Pinto::Server, which does send the output over a pipe, it doesn't know if you're piping that somewhere else or not.

I don't remember exactly, but I'll figure it out.

Contributor

mar-kolya commented Apr 24, 2014

Yeah, I know about -D and -P. In my particular case I had a special format output and was grepping by that format, but color was getting in the way.

I think my patch works correctly with Server - I've tested that. That's why I moved that code out of 'Pinto::Chrome::Term'.

Owner

thaljef commented Apr 24, 2014

I think I'll combine --no-color and --color into one negatable option called color with default 1.

thaljef pushed a commit that referenced this pull request Apr 25, 2014

Jeffrey Ryan Thalhammer
Disable color when piping or redirecting. Fixes GH #155.
Also, changed command-line options so you can enable or
disable colors with --color or --no-color.

thaljef pushed a commit that referenced this pull request Apr 28, 2014

Jeffrey Ryan Thalhammer
v0.09992_01
      [INCOMPATILBE CHANGES]

      - The protocol used to communicate with a remote Pinto repository is now
      versioned with media types via the "Accept" header.  So if you upgrade
      to this version of pinto, you'll also need to upgrade pintod to this same
      version.  Going forward, the protocol will be versioned so that we
      can potentially support different versions of the client and server at
      the same time.  But for the time being, you'll get an exception telling
      you whether to upgrade your client (pinto) or the server (pintod).

      - The PINTO_COLORS and PINTO_COLOURS environment variables are deprecated
      in favor of PINTO_PALETTE.  You can still use the old names for now, but
      they will be removed in a future release.

      - When using the --all option with the "list" command, the default format
      shows the pin status as "?" (i.e. indeterminable) instead of showing the
      main module status.

      [BUG FIXES]

      - Colorization is now disabled by default when the output is being piped or
      redirected to a file.  This improves interoperability with tools like grep,
      sort, etc.  To force colorization, use the --color or --colour option.
      Fixes GH #155. Thanks @mar-kolya.

      - The "roots" will now include test and develop prereqs, since Pinto has no
      way of knowing how those distributions relate to your application. This
      fixes GH #158. Thanks @mar-kolya.

      [ENHANCEMENTS]

      - When using the "add" command, the paths to the local archives can now be
      expressed using file:/// URLs.

      - The PINTO_PAGER_OPTIONS environment variable can now be used to pass specific
      options to your pager when using it with pinto.  For example, to tell `less`
      to display colors.

      [DOCUMENTATION]

      - Revised the Tutorial and QuickStart documentation.

thaljef pushed a commit that referenced this pull request May 3, 2014

Jeffrey Ryan Thalhammer
v0.09993
      [INCOMPATILBE CHANGES]

      - The protocol used to communicate with a remote Pinto repository is now
      versioned with media types via the "Accept" header.  So if you upgrade
      to this version of pinto, you'll also need to upgrade pintod to this same
      version.  Going forward, the protocol will be versioned so that we
      can potentially support different versions of the client and server at
      the same time.  But for the time being, you'll get an exception telling
      you whether to upgrade your client (pinto) or the server (pintod).

      - The PINTO_COLORS and PINTO_COLOURS environment variables are deprecated
      in favor of PINTO_PALETTE.  You can still use the old names for now, but
      they will be removed in a future release.

      - When using the --all option with the "list" command, the default format
      shows the pin status as "?" (i.e. indeterminable) instead of showing the
      main module status.

      [BUG FIXES]

      - Colorization is now disabled by default when the output is being piped or
      redirected to a file.  This improves interoperability with tools like grep,
      sort, etc.  To force colorization, use the --color or --colour option.
      Fixes GH #155. Thanks @mar-kolya.

      - The "roots" will now include test and develop prereqs, since Pinto has no
      way of knowing how those distributions relate to your application. This
      fixes GH #158. Thanks @mar-kolya.

     - Fixed timezone.t test which would fail in some cases, ostensibly
     because the test environment would not allow us to open sockets
     (probably due to security reasons).  Thanks CPAN Testers.

      [ENHANCEMENTS]

      - When using the "add" command, the paths to the local archives can now be
      expressed using file:/// URLs.

      - The PINTO_PAGER_OPTIONS environment variable can now be used to pass specific
      options to your pager when using it with pinto.  For example, to tell `less`
      to display colors.

      [DOCUMENTATION]

      - Revised the Tutorial and QuickStart documentation.
Owner

thaljef commented May 3, 2014

This has shipped to CPAN and Stratopan as Pinto 0.09993

@thaljef thaljef closed this May 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment