abrochard / kubel

Emacs extension for controlling Kubernetes with limited permissions
GNU General Public License v3.0
259 stars 41 forks source link

Use faces to propertize status #99

Closed PuercoPop closed 1 year ago

PuercoPop commented 1 year ago

Currently we are hard-coding arbitrary colors for the statuses. With the theme ef-summer the contrast with chosen colors is not great. This PR lets the user customize the colors to fix the problem on their.

image

Ideally I would like to suggest the use of either built-in or newly defined-faces for this purpose, but I wanted to ask you what if you'd like that before I included that change. f/e we can replace (:foreground "red) with the built-in error face, green one with success and the orange coloured one with warning. That way it would be more likely that kubel plays well with the user's theme

abrochard commented 1 year ago

Thank you for proposing this, I love it! I am not very familiar with faces in general but I think using the built-in faces makes sense. As you said:

That leaves completed/yellow and terminating/blue. Are there any built-ins that could match? Or we just leave them with a simple color? As long as people can override to tune to their theme, it should be ok. Also maybe "Completed" should actually be green?

PuercoPop commented 1 year ago

@abrochard Ok, I've done the changes. I went head and defined a face for each status. That way people can group them as they see fit. I've also used the built-in faces instead of the current colors were there is a clear choice (the one you listed above). For the others I'm not sure which built-in would be applicable if any so I left them as is. Personally I'm using transient-amaranth for completed, but that is just my personal preference.

Also maybe "Completed" should actually be green?

No, I think it makes sense to distinguish it. completed pods are mostly metadata afaiu. I can view their logs but I can't exec into them. If anything something like comment may be more suited. But for now I left it as is.

LMKWYT

abrochard commented 1 year ago

Thank you! I tested locally and it looks good. I agree with the choices. One last thing and then we're good to merge: the linter is complaining that docstring sentences should end with punctuation, in reference to the kubel-status-* docstrings.

PuercoPop commented 1 year ago

@abrochard Updated. Curious what linter are you using? Is it built in? I have a a couple of more changes that to silence the byte-compiler but non pointed out the issue the missing period, which is indeed an elisp convention. This are the warnings I get when byte-compiling kubel.el right now:

In kubel--column-entry:
kubel.el:396:8: Warning: docstring wider than 80 characters

In kubel--get-command-prefix:
kubel.el:569:8: Warning: docstring wider than 80 characters

In kubel--jump-back-to-line:
kubel.el:674:9: Warning: ‘goto-line’ is for interactive use only; use
    ‘forward-line’ instead.

In kubel-apply:
kubel.el:698:35: Warning: assignment to free variable ‘dir-prefix’
kubel.el:701:48: Warning: reference to free variable ‘dir-prefix’

In kubel-exec-vterm-pod:
kubel.el:1007:4: Warning: misplaced interactive spec: ‘(interactive)’

In end of data:
kubel.el:414:21: Warning: the function ‘kubel--get-column-entry’ is not known
    to be defined.
abrochard commented 1 year ago

Thank you! Merging now. I'm just using the basic flycheck setup for elisp I believe. I should make a note to tackle those warnings.