zsh-syntax-highlighting

zsh-syntax-highlighting copied to clipboard

I'm not yet happy with loading a local theme. If a user sets ZSH_HIGHLIGHT_THEME=error-only and then happens to have a file in their current directory called error-only, it will pick that up.

I'm not yet happy with loading a local theme. If a user sets ZSH_HIGHLIGHT_THEME=error-only and then happens to have a file in their current directory called error-only, it will pick that up.

So check that [[ ${~ZSH_HIGHLIGHT_THEME} == /* ]]?

I had thought of that, but what about people who want to use ~? Are there any other prefixes or does checking /* and ~* suffice?

How about allowing $ZSH_HIGHLIGHT_THEME to be a function name? I think this could be implemented in two ways, either by if $ZSH_HIGHLIGHT_THEME; (( $? != 127 )); then (where 127 is the exit code of "No such command", at least on my system), or by checking $functions, which requires zmodload zsh/parameter, but we have plenty use-cases for that module, and it's probably available? (Is there a distro that doesn't compile all the default modules, or an environment where using a module is expensive?)

I had thought of that, but what about people who want to use ~? Are there any other prefixes or does checking /* and ~* suffice?

When would you get a literal tilde there? If you do x=~/foo then [[ $x[1] == / ]] after the assignment. Is the concern x="~/foo" (quoted)?

Since I used GLOB_SUBST (through the ${~foo} syntax), literal tildes would be expanded. Although, come to think of it, asterisks would be expanded too, so perhaps a plain old [[ $x == /* ]] is the right way to go after all?

There's nothing besides slash and tilde. (There's the EQUALS syntax, but it's not applicable here)

Right. I forgot ~ expansion happened at assignment. Updated PR.

Nice, thanks. When you write docs for this (which I accept doesn't need to be now — design is still in flux), it might be good to note that $ZSH_HIGHLIGHT_STYLE_DEFAULT and $zle_highlight[default] are not the same thing.

Summarizing my discussion points from IRC:

• Have source /path/to/themes/default.zsh abstracted into a _zsh_highlight_load_theme default
• Support themes in .zshrc and in /provided/path/to/file/to/be/sourced
  • To this end, consider allowing $ZSH_HIGHLIGHT_THEME to be a function name,
• Support themes that either replace default.zsh or build on top of it

Forward compatibility: a theme written for 0.5.0 might not define styles introduced in 0.6.0. Ensure the design handles this.

As an extreme example, an empty theme (simply touch themes/empty.zsh; ZSH_HIGHLIGHT_THEME=empty) should work. See that either the keys of ZSH_HIGHLIGHT_STYLES[] are set, or the highlighters cope with missing keys.

Turns out coping with missing keys is mostly an issue in the tests. Updated branch where make quiet-test tests all themes and succeeds; although it shouldn't because exec-redirection 4 is XFAIL while it should be XPASS for error-only.

Less stupid workaround for TODO tests. Test exec-redirection 4 now is correctly identified as XPASS and fails make test. This seems like a good reason to make XPASS not fail tests as the issue is not resolved, but it won't show in the error-only theme.

Once that's merged, we could migrate the test suite to run under a theme that has ZSH_HIGHLIGHT_STYLES as the identity mapping. Or, as Matthew points out, at least an injective mapping.

The branch has a merge conflict due to 30d8f92df2258c263185ede09925fef3fa47019c on master.

Once this is merged, ~~I~~we should probably add a screenshot of the error-only theme to README.

[The existing screenshot uses Inconsolata 24; the new screenshot should probably do the same for readability when the two screenshots are juxtaposed.]

Updated commits. I believe all that is left is

• Fixing make test. Currently it fails for the error-only theme since it is a very non-injective mapping and XPASS is a failure. I think all themes should be tested as this caught the brackets problem. Perhaps don't fail on XPASS for most themes, but have a special injective theme for which XPASS is an error.
• Writing docs.
• Adding a screenshot.

Did I miss anything?

Do you want to cherry-pick d8411a6 to master now? It's a flag day change, so we'll both have headaches down the road if tests are changed on master before that patch is merged.

In _zsh_highlight_load_theme, please declare local theme. (The driver sets warncreateglobal, but I guess that function is outside the setopt's scope.)

About XPASS with error-only: it's a bug in the test case or in the test harness. I've filed a separate issue for that (#287). As I say there, I think the proper way to fix this would be to run the test suite just once, with an injective $ZSH_HIGHLIGHT_STYLES mapping (instead of f953d773107d76234858a673f378d0525bb07ebd). So I think we can fix #287 as part of the themes branch: we'd just need to add an identity-mapping theme, that's thirty seconds of work so won't delay the themes branch.

Nothing else comes to mind.

Perhaps we should solicit feedback from carstenh? I'm thinking of user-level feedback (focus group), not of implementation-level feedback.

In _zsh_highlight_load_theme(), how about changing source $foo to source $foo --, to make it easier to extend the driver-to-theme interface in the future with positional arguments?

afe869ddc0a5491c42008039d2ec66c8a45ac6e8 needs to use ${(q-)}.

Thanks for following the log messages convention :)

Remaining items are those from today's and yesterday's comments.

As the code stands, a missing key will result in https://github.com/zsh-users/zsh-syntax-highlighting/blob/31ac2b36a9156ef424e8ccee560ec31f97bd3eef/highlighters/main/main-highlighter.zsh#L80 being executed with [[ $style == '' ]], won't it?

That would append an invalid value to $region_highlights: an entry of the form"1 5 " (with no "highlighting specification" part). Instead, we can avoid adding a $region_highlight entry, or add an entry with a none style (not sure yet which is better).

As the code stands, a missing key will result in https://github.com/zsh-users/zsh-syntax-highlighting/blob/31ac2b36a9156ef424e8ccee560ec31f97bd3eef/highlighters/main/main-highlighter.zsh#L80 being executed with [[ $style == '' ]], won't it?

This is also discussed in https://github.com/zsh-users/zsh-syntax-highlighting/issues/287#issuecomment-204843028 et seq, including a proposed test suite patch.

Forward compatibility: how should we handle the case of a theme written for 0.5 and used with 0.6, where 0.6 defines new token classes ($ZSH_HIGHLIGHT_STYLES keys)?

edit: Such as the arg0-$kind case of #316.

Other things to consider are how third party highlighters can extend themes and how third-party themes can be included. We could have

ZSH_HIGHLIGHT_THEME_DIRS=(${0:A:h}/themes highlighters/*/themes/)

and source $^ZSH_HIGHLIGHT_THEME_DIRS/$theme so that

1. Third-party highlighters may extend themes and
2. Third-party themes may be added by appending to the array.

Since themes sourced later override themes sourced prior, third-party themes may override decisions we or third-party highlighters make.

1. With the proposed implementation, highlighters will only be able to install hooks for explicitly-enumerated themes. I'd rather enable highlighters to have if [[ $theme == (foo|bar|bazv<1-3>) ]] style handling in their "post-theme hook". This could be achieved by defining an entry point _zsh_highlight_${highlighter}_post_themes (which could be passed the theme as an argument). (Or possibly via zstyles?)

2. Why is a "post-theme hook" needed in highlighters? A highlighter can implement theme-dependent behaviour by checking $ZSH_HIGHLIGHT_THEME; a theme can implement highlight-dependent behaviour by checking $ZSH_HIGHLIGHT_HIGHLIGHTERS¹.

   Logically, highlighters answer "What?" and themes answer "How?". (However, I'm not sure what API this observation indicates.)

3. What happens if $ZSH_HIGHLIGHT_THEME_DIRS contains two copies of a theme? What if the two copies are from different releases of the theme? Can we design the API to impose fewer requirements on theme authors?

4. I don't get "Third-party themes may be added by appending to the array". Installing a third-party theme is already possible by setting $ZSH_HIGHLIGHT_THEME to an absolute path, so appending to the array will only let you install post-hooks for a specific theme (additional files named, for example, ${ZSH_HIGHLIGHT_THEME_DIRS[-1]}/error-only). If that is the goal, I'd rather have _zsh_highlight_load_theme do elif (( ${+functions[_zsh_highlight_hook__theme]} )) && _zsh_highlight_hook__theme ${theme}, then the hook can do whatever customizations it wishes (either before or after sourcing the theme file proper), and creating a separate file for the customization is not required.

5. I'm confused by the singular/plural mismatch between _zsh_highlight_load_theme (variadic), $ZSH_HIGHLIGHT_THEME (singular), and $ZSH_HIGHLIGHT_THEMES (was present in previous iterations of the patch).

¹ Although I imagine in most cases a theme would simply set $ZSH_HIGHLIGHT_STYLES[myhighlighter:aurora] without bothering to check whether myhighlighter is enabled or even installed.

Any update on when/if this wll be merged?

Part of the wait is undecided questions (e.g. how should third party themes be handled). Part is also wanting to hear some feedback from users to see if this implementations fits the common use cases before committing to an API. If you have any feedback on the implementation, interface, or docs; I'd love to hear it.

@phy1729 Oh dear, I'm just gonna have to put my money where my mouth is and test it, won't I? ☺

Expect more feedback later on today… Or right now:

(themes|✓); git up
Fetching origin
remote: Counting objects: 6, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 6 (delta 4), reused 6 (delta 4), pack-reused 0
Unpacking objects: 100% (6/6), done.
From https://github.com/zsh-users/zsh-syntax-highlighting
 + 32d15ed...200dfa6 themes     -> origin/themes  (forced update)
fatal: Not possible to fast-forward, aborting.
(themes↓4↑1|✓); 

Should this matter?

Am I right in thinking that unless I use the default theme, I will need to create my own? I can attempt a zenburn or solarize one as that might interest other people not just my-wierd-and-bizzar colours theme.

There is a strange error when I use tab to complete a file name inside a command. If I do:

intranet(feature/398↑1|+1); kubectl -v=5 create -f mariadb<Hit TAB here>

This is what happens:

intranet(feature/398↑1|+1); kubectl -v=5 create -f mariadb__handle_flag:2[0/540]math expression: operand expected at `'                                         
b-        

Note that this does not happen all the time. On some commands, it works fine. It could be if there is no obvious choice,

Am I right in thinking that unless I use the default theme, I will need to create my own? I can attempt a zenburn or solarize one as that might interest other people not just my-wierd-and-bizzar colours theme.

Currently there are two themes (default and error-only). If you want to use other colors you can define them in your zshrc directly (already possible without this PR) or define a theme. When defining a theme, you'll be able to inherit from another theme and override only what you change.

We'd love a solarized theme, please let us know if you write one! :-)

@kierun Re kubectl, could you confirm that that happens with the themes branch installed and not without it?

@\all: Some additional thoughts from the brainstorming are being added as separate issues at https://github.com/zsh-users/zsh-syntax-highlighting/labels/feature:themes.

@kierun Re kubectl, could you confirm that that happens with the themes branch installed and not without it?

@danielshahaf Yes, it is with the theme branch.

@kierun Could you please also confirm that it does not happen when you use the master branch? Thanks. (And feel free to join us on #zsh-syntax-highlighting on freenode, if you prefer.)