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

fix: check_colors #119

Merged
merged 2 commits into from
Feb 17, 2022
Merged

fix: check_colors #119

merged 2 commits into from
Feb 17, 2022

Conversation

habamax
Copy link
Collaborator

@habamax habamax commented Jan 16, 2022

* Use vim9script for convenience
* Close #31
* Close #35
@habamax habamax requested a review from romainl January 16, 2022 11:41
@romainl
Copy link
Collaborator

romainl commented Jan 16, 2022

Are we sure check number 4 (&t_Co) is really necessary?

@habamax
Copy link
Collaborator Author

habamax commented Jan 17, 2022

Are we sure check number 4 (&t_Co) is really necessary?

Probably not -- I have just ported existing one without analyzing whether they are needed or not

@habamax
Copy link
Collaborator Author

habamax commented Jan 17, 2022

Does it make sense to have check against what we expect from the colorscheme generated with colortemplate?

  1. it should have all needed highlight groups for both light and dark backgrounds if both are used
  2. it should have definitions for gui, 256 and 16 colors
  3. terminal colors (g:terminal_ansi_colors)
  4. ...?

@romainl
Copy link
Collaborator

romainl commented Jan 17, 2022

About check 4…

Colorschemes generated via colortemplate are one scenario but it is very reasonable, I think, to assume that the script will be run against handwritten colorschemes that may or may not have all the bells and whistles. A GUI-only colorscheme without &t_Co checks may not pass our filter but it could nonetheless be a perfectly valid colorscheme.

Maybe we can split: Errors vs Warnings?

@habamax
Copy link
Collaborator Author

habamax commented Jan 17, 2022

Depends on the usecase: whether we only need this check_colors for our repo and whether we require both gui and at least 256c fallback...

@romainl
Copy link
Collaborator

romainl commented Jan 17, 2022

IMO, if check_colors is only meant for this repo, then it should be optimised for strictness. If we want it to be useful outside of this repo, then some rules may be "lightened". Here is how I see it:

Errors in all scenarios:

  • colorscheme name
  • highlight groups
  • background
  • init
  • syn on
  • normal

Those checks are purely technical and are not affected by/don't compromise creativity.

Warnings outside of this repo:

  • GUI, 256c, 16c
  • &t_Co, because a GUI-only or 16c-only colorscheme is perfectly acceptable outside of this repo.
  • terminal colors, because it is only theoretically honored in GUI or with termguicolors so it does nothing in a 16c/256c scenario

Those checks are also technical but they affect/can be affected by creative choices.

Now is it a direction we are comfortable with? On one hand that would make the script useful for people who don't intend to submit their colorscheme for approval. On the other hand that would make it more complex and less useful for us.

A possible middle ground would be to make every check mandatory while prepending some header to the output that puts a few things in perspective.

@habamax
Copy link
Collaborator Author

habamax commented Feb 17, 2022

Is it ok to merge?

@romainl romainl merged commit 7b49ca8 into master Feb 17, 2022
@romainl
Copy link
Collaborator

romainl commented Feb 17, 2022

What about the g:terminal_ansi_colors check? Do you already have an idea of how you want to do it?

@habamax
Copy link
Collaborator Author

habamax commented Feb 17, 2022

What about the g:terminal_ansi_colors check? Do you already have an idea of how you want to do it?

I will have to think about it

@habamax habamax deleted the fix/check_colors branch February 18, 2022 12:10
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.

check_colors.vim does not check LineNrAbove/LineNrBelow Remove checks number 7 and 8 from check_colors.vim
2 participants