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

lib/appearance: export $CLICOLOR instead of $LSCOLOR #2028

Merged
merged 4 commits into from
Mar 4, 2022

Conversation

gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Jan 8, 2022

Description

  • Export $CLICOLOR, instead of $LSCOLORS, and add a comment to the base theme to help theme authors;
  • shellcheck;
  • Alsö, since the value of $CLICOLOR is not used anywhere, overload it to count the number of colors available for use elsewhere.

Motivation and Context

Exporting $LSCOLOR from lib/appearance has always been weird to me and it took me a few rounds to figure out why: it's not useful. BSD ls enables color output if $CLICOLOR is set (to any value, even blank), and GNU ls doesn't use any variable to enable color listings. (GNU ls requires --color=auto or similar.) The $LSCOLORS (BSD) and $LS_COLORS (GNU) variables do not enable color listings; they just specify which color scheme to use (with incompatible formats).

Themes can and should customize $LSCOLORS/$LS_COLORS, but this should not be hard-coded by the main files. Both GNU and BSD ls have default colors they use when these variables are not set.

Alsö resolves #2082.

How Has This Been Tested?

This is part of my main branch, and I've tested to make sure it loads, and the test suite passes.

Types of changes

  • Weird fix (non-breaking change which calms the brain worms)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@gaelicWizard gaelicWizard marked this pull request as ready for review January 8, 2022 23:01
@gaelicWizard gaelicWizard force-pushed the plugin/base branch 2 times, most recently from 816860b to 6d4ba6b Compare January 9, 2022 09:02
@tsiflimagas
Copy link
Contributor

Something related to the code touched here is that LSCOLORS is used by the BSD ls, while the GNU one uses LS_COLORS, which also uses a different format to define colours. I guess that should be set too. The dircolors command outputs a basic set of colours to use, and its output could even be used with eval (well, I guess we'd prefer to just include the variable). What do you think?

@gaelicWizard
Copy link
Contributor Author

Something related to the code touched here is that LSCOLORS is used by the BSD ls, while the GNU one uses LS_COLORS, which also uses a different format to define colours. I guess that should be set too. The dircolors command outputs a basic set of colours to use, and its output could even be used with eval (well, I guess we'd prefer to just include the variable). What do you think?

Yes, that all sounds like we should address. I would have to spin up a Linux to play with GNU ls. I feel like maybe a plugin/ls is in order 😆

@gaelicWizard gaelicWizard force-pushed the plugin/base branch 2 times, most recently from 6e9835a to 5b5066b Compare January 17, 2022 06:05
@gaelicWizard
Copy link
Contributor Author

I found that theme/clean had a generic setting for $LS_COLORS (and a duplicate for $LSCOLORS), so I moved that here now too.

@gaelicWizard gaelicWizard changed the title plugin/base: move $LSCOLORS here plugin/ls: new plugin for $LSCOLORS && $LS_COLORS Jan 17, 2022
@gaelicWizard
Copy link
Contributor Author

I made plugin/ls 😃

Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

This is a slightly breaking change, as users will have to enable the plugin so things will work as expected. I am okay with it though

@gaelicWizard gaelicWizard force-pushed the plugin/base branch 5 times, most recently from 4f207d3 to ca1113c Compare January 31, 2022 20:45
@gaelicWizard gaelicWizard marked this pull request as draft February 6, 2022 11:28
@gaelicWizard
Copy link
Contributor Author

I'm sorry, I've wrapped around again and I don't think this should be it's own plugin anymore. I think this should be in lib/theme. It's really not a plugin, but actually theme-dependent color scheme (at least for GNU ls). I'll update this PR this weekend.

@gaelicWizard gaelicWizard changed the title plugin/ls: new plugin for $LSCOLORS && $LS_COLORS lib/appearance: export $CLICOLOR instead of $LSCOLOR Feb 7, 2022
@gaelicWizard
Copy link
Contributor Author

Ok guys, totally re-did this branch. Can you check it out again?

Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

Is this ready @gaelicWizard ?
I am okay with merging it

@gaelicWizard
Copy link
Contributor Author

👍

Alsö, since the *value* of `$CLICOLOR` is not used anywhere, overload it to count the number of colors available for use elsewhere.
@NoahGorny NoahGorny merged commit 49649c5 into Bash-it:master Mar 4, 2022
@gaelicWizard gaelicWizard deleted the plugin/base branch March 4, 2022 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants