Skip to content
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

RFC: use LS_COLORS for coloring the filenames in result list #191

Open
trapd00r opened this issue Mar 21, 2019 · 32 comments

Comments

Projects
None yet
3 participants
@trapd00r
Copy link

commented Mar 21, 2019

While acking through a codebase with lots of different filetypes, it can
be hard to differentiate them from each other.

Adding a bit of familiar colorization to the filenames help
tremendously. I made a quick hack in ack2 to do just that; what do
you think?

ack

@petdance petdance added the feature label Mar 21, 2019

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

Do any other tools do this sort of thing?

@n1vux

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

From the top of my head:

  • ls(1)
  • vdir
  • vidir - file manager inside of vim
  • most tui file managers

The idea is that since you're already so familiar with your own dircolors settings (how many times do you run ls a day?), getting an overview of what kind of files you're dealing with at the moment will be much easier.

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

I don't understand how these are being colored
I infer he has a patch to ack3 that uses his module. I would use this .

I made a proof of concept hack in ack2 and merely checking the interest among you guys.

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

My comment about about "I don't understand how these are being colored" was not about the code. I didn't understand about dircolors. Now I sort of understand.

The idea is that since you're already so familiar with your own dircolors settings (how many times do you run ls a day?),

I've never thought about dircolors or LS_COLORS until today. I'm wondering how many folks would use this.

Certainly, using LS_COLORS would not be a default behavior. The user would have to use a --ls_colors option, probably kept in .ackrc.

I'll also point out that ack has a number of design principles spelled out in DESIGN.md, including:

  • ack must run purely as Perl 5.10.1 using only core modules and File::Next.

No other modules may be used. That includes Moose or any Moose variants.

  • We don't use the Perl smartmatch operator.

  • ack must be able to be distributed through CPAN, using the App::Ack:: module tree.

  • ack must be able to be built as a single-file standalone file.

This file is intended to be able to be copied & pasted if necessary.
It will not contain any characters out the range of [ -~].
See xt/coding-standards.t.

  • ack must be cross-platform. Specifically, it must run on Windows.

  • ack must not use any external tools.

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

Got it. Closing.

@trapd00r trapd00r closed this Mar 21, 2019

@petdance petdance reopened this Mar 21, 2019

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

I'm sorry if I sounded like we weren't interested. I'm just laying out constraints.

@n1vux

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

I'm also wondering if it might not be too horrible if we would let --ls_colors use File::LsColors iff it was installed like a regular Perl module.

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

The proof of concept I made did just change a single line: trapd00r/ack2@9af0270#diff-82d7ba7ea655a2bbde5a4e2153a66daeR432

obviously it would need more work than this, but I just wanted to see what it would look like and if it did indeed enhance my experience.

@n1vux here you go: https://github.com/trapd00r/LS_COLORS :)

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

I just wanted to see what it would look like and if it did indeed enhance my experience.

Did it?

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

Very much so.

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

What's in your ~/.ackrc, @trapd00r ?

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

--type-set=xresources=.Xresources,.Xcolor
--type-set=stumpwm=.stump,.stumpdump,.dump
--flush
--follow
--nobreak
--sort-files

@n1vux

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

I'm also wondering if it might not be too horrible if we would let --ls_colors use File::LsColors iff it was installed like a regular Perl module.

Yes, I was thinking on the Metro (T) home and was about to suggest the very same as the DRY alternative, and here it is already for me to agree with.
An Optional module. (Whether we'd recommend it be packaged as what Debian calls Suggests, IDK; that might be too strong, especially with the following option.)

(In which case we could even not require -ls-colors in .ackrc but take installation of File::LS_COLORS on @INC and setting of $ENV{LS_COLORS} as sufficient DWIM indication, and list $ENV{LS_COLORS} as used if set and if optional module installed?)

Making it optional would require xt tests that test that it is ignored if omitted and used if provided, either as unit tests with a mock File::LS_Colors and/or with build-requires of File::LS_Colors for integration test.

Color me interested.

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

For those who don't have --nobreak, where we see the matches grouped by file, would we still color-code the files per LS_COLORS?

The use of LS_COLORS has to be an explicit thing. Otherwise someone installing File::LsColors would change ack's out from under you. Also, I have LS_COLORS set even though I haven't done it. Must be something in my Linux distro.

@n1vux

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

The use of LS_COLORS has to be an explicit thing.
Otherwise someone installing File::LsColors would change ack's out from under you.

It it was installed explicitly that might be good enough but it could become a prerequisite to something else, in which case yeah, ok, can't take a joint implication as explicit request. You've convinced me.

Also, I have LS_COLORS set even though I haven't done it. Must be something in my Linux distro.

Oh, right, so it does. (I typo'd it first time i tried it. )

@n1vux

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

For those who don't have --nobreak, where we see the matches grouped by file, would we still color-code the files per LS_COLORS?

We do color the filename headline in the --group --break case.

I see no reason not to apply --ls-colors if .ackrc + @Args combine to --ls-colors --group --break; the user has the choice.

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

What do we do if someone has --ls_colors set and there is no optional File::LsColors?

@n1vux

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Having looked at the code, yes, optional use-iff-loaded-and-requested of File::LsColor module is much preferable than trying to cut-n-paste reuse it.

just change a single line: ...
obviously it would need more work than this

yes, but the rest of the work is housekeeping, conditional-on-options, and testing; it's straightforward to use your module iff requested and available.

( the hardest part is doing the regression tests to make sure
(1) it doesn't use/require module if not requested
(2) it warns not dies if --ls-color is requested but not available
(3) color of output changes if requested
(4) color of output changes even more if LS_COLORS changes
:-D.
Since i'm doing Perl Test coaching on day job now, i can help with that if Andy doesn't do it first.
)

@n1vux

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

What do we do if someone has --ls_colors set and there is no optional File::LsColors?

I'd say either Warn, or Die politely (user centered diagnostic message) instead of letting Perl do the vanilla die with path dump.

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

Let's put a fork in implementation discussions for now. Right now I'm more interested in 1) what other tools do and 2) trying to gauge how many folks would use it, if it's worth the significant amount of code weight it would add.

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

Trying to get a sense of who would use it.

https://twitter.com/BeyondGrep/status/1108800847402672131

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

@trapd00r What would you think if we just colorized based on extension and didn't bother with things like checking for +x and so on?

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

I think it would make sense, not having to stat each file.

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

I could make it configurable if it would help, e.g. something like $File::LsColor::no_stat. It would be a good idea anyway.

@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 22, 2019

Here's my current thinking about the idea:

We need to have a way to customize colors based on filetypes, as ack understands them, which is more than just extensions and names.

We currently define types like so:

--type-add=perl:ext:pl,pm,pod,t,psgi
--type-add=perl:firstlinematch:/^#!.*\bperl/
--type-add=perltest:ext:t

And then we could logically add:

--type-add=perl:color:38;5;9

or

--type-add="perl:color:bold green on_yellow"

or

--type-add=perl:color:rgb404

Once we have that all working, then we would have one of two choices (or maybe others):

  1. Add a mode where ack reads the colors from LS_COLORS and translates appropriately, or

  2. Provide some way to convert LS_COLORS to --type-add arguments that you could add to your .ackrc.

I'm liking the idea of colorizing filenames based on file type a lot. I just think that we have to do it the ack way, and not the ls way.

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

Wouldn't it be much simpler for a user to have to configure colors only
one time? If user would want to color all perl-related files the same
way, doing it in LS_COLORS is very trivial. Personally I wouldn't; I want
to easily distinguish pm from pl from t files.

I can even distinguish pl from Makefile.PL files.
2019-03-22-092405_238x114_scrot

Furthermore I still believe that actually using LS_COLORS gives many benefits:

  1. Files will always look the same across programs that support it
  2. When a user would want to change his colors, he'll do it at one place and it'll instantly reflect everywhere else (once he starts a new shell).
@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 22, 2019

0.499 2019-03-22
  - introducing $File::LsColor::NO_STAT variable. If set, no stat() will be
    made. This can be desired if the filenames aren't real files, or for
    performance reasons.
@petdance

This comment has been minimized.

Copy link
Collaborator

commented Mar 22, 2019

It's not going to be an either-or of ack-style definitions or LS_COLORS. I want it to be both.

The big thing is that the idea of filetypes in ack doesn't match how LS_COLORS work. File-detection in ack can be more comprehensive than how LS_COLORS checks extensions.

@trapd00r

This comment has been minimized.

Copy link
Author

commented Mar 26, 2019

Probably not good enough, but here it is implemented in ack3
ack3_lscolor

@n1vux

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Probably not good enough

It's a start, thank you.

The exciting-but-boring part is writing xt/ tests for TravisCI etc, to test both with and without File::LsColor.
It should be possible to fake not having an optional module for author-tests but I don't remember seeing an idiom for that yet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.