allow to set semantic highlight groups based on filetype #4167
Conversation
|
Thanks for sending a PR! Just so I'm clear, what was the specific use case that motivated this? Why would a user want to do this in general and specifically? One question about this implementation - is it a breaking change? Ie does it make existing setups no longer work as they did before? I realise that semantic highlighting is experimental still but I don't like to break existing setups if I can help it. |
Sorry, I thought that it is obvious. So:
Except that there is a problem (in current version) with disabling some highlight groups or using specific highlight groups, that are defined for specific file types. Both cases are handled in that pr
I don't think so
If you defined some special highlighting previously, for example for functions, then it will not used anymore. Instead it will use default highlight. But you will not have any error or something like that |
This is exactly what I mean by existing configurations no longer working. After upgrade it no longer behaves as it did and use must somehow divine the reason why and will likely just raise a bug. I have custom config myself and I wouldn't expect/want it to stop working. |
|
I thought that it is not a problem, because feature is experimental. Anyway, I updated pr a little bit, so now that settings will be used as default |
|
Great thanks. I'll try to give this a spin later. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4167 +/- ##
===========================================
- Coverage 89.70% 78.60% -11.10%
===========================================
Files 34 34
Lines 4420 4469 +49
===========================================
- Hits 3965 3513 -452
- Misses 455 956 +501 |
|
@puremourning any updates on that? |
|
Sorry tau take me a while to get round to testing/checking this as I have some stuff on at the moment. Feel free to ping me weekly ! |
There was a problem hiding this comment.
Thanks for this. The logic for determining the set of groups seems rather fiddly. I would have expected something akin to groups_by_filetype.get( current_filetype, defaults), though I realise it may be more complicated than that, is there a way to simplify at all?
Otherwise, I do think that the logic is now sufficiently complicated that we should consider some sort of test for it. There are 2 choices, pure python unit test in python/ycm/test, or preferably full-vim integration test in tests/
Reviewable status: 0 of 2 LGTMs obtained (waiting on @andrejlevkovitch)
python/ycm/semantic_highlighting.py line 72 at r3 (raw file):
f"hlexists( '{ vimsupport.EscapeForVim( hi ) }' )" ): vimsupport.PostVimMessage( f"Higlight group { hi } is not difined"
I wonder if we can provide more context for this? The user might not actually know why they are seeing this. It's not unusual for users to just copy/paste stuff off stack overflow or whatever and not really know why they are seeing a particular error. If we can say something like "Hilight group {x} set for filetype {y} is not defined. Please see :help ....". Only bother if it's easy.
python/ycm/semantic_highlighting.py line 73 at r3 (raw file):
vimsupport.PostVimMessage( f"Higlight group { hi } is not difined" )
nit: put the ) on the previous line to be consistent with prevailing style.
At worst, indent it aligned with vimsupport
python/ycm/semantic_highlighting.py line 100 at r3 (raw file):
hi_groups: list[dict] = vimsupport.VimExpressionToPythonType( "g:ycm_semantic_highlight_groups" )
as above, this style of close parent isn't our style.
python/ycm/semantic_highlighting.py line 101 at r3 (raw file):
"g:ycm_semantic_highlight_groups" ) hi_groups.extend( HIGHLIGHT_GROUPS[:] )
I think this will actually change the value of g:ycm_semantic_highlight_groups. Intentional?
python/ycm/semantic_highlighting.py line 140 at r3 (raw file):
super().__init__( bufnr ) self._filetypes = vimsupport.CurrentFiletypes()
It's not guaranteed that the filetypes of bufnr are the same as the "current" filetypes. We should get the filetypes for bufnr
python/ycm/semantic_highlighting.py line 148 at r3 (raw file):
for filetype in self._filetypes: if filetype in ft_groups[ 'filetypes' ]: target_groups = ft_groups
should we break out here? if filetypes are duplicated, we should provably use the first such list? or should we merge them? I think here we're always using the last such defined set, which means the defaults (in HIGHLIGHT_GROUPS), if set for a filetype would override user settings.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @andrejlevkovitch)
IDK
Sorry, but I am not python developer and I don't know the codebase well, so, probably, I can not do that
Hm, I think that should not change variable in vim, but I can be wrong. Anyway, I don't think that this is a problem as that variable we use only here
Yeah, that makes sense
Yeah, that would be better, I wanted to do that at first, but there are no
yeah, that is right, but I don't think that we should set defaults per files - lets leave it for user |
|
made some fixes, please take a look |
|
@puremourning were you able to check the latest changes? |
|
@puremourning Hi! Please take a look |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @andrejlevkovitch)
a discussion (no related file):
So I tried this out. After reading the docs, I added this to my config:
let g:ycm_semantic_highlight_groups = [
\ {
\ 'highlight': {
\ 'Keyword': 'WarningMsg'
\ }
\ }
\ ]
The result was that semantic highlighting no longer works and I get:
Token type macro is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type variable is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type function is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type parameter is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type class is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type comment is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type type is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type operator is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type property is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type enumMember is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type label is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
Token type enum is not defined for ['c']. See :help youcompleteme-customising-highlight-groups
So it seems that if you specify an entry in this list, you must supply a mapping for all LSP token types. To me, this seems both unintuitive and breakable. Users can't be expected to update these lists every time a new one is supported by YCM or a server.
I think a much more common use case would be as above - just changing one or other HL group rather than overwriting every one.
I would much prefer it to work as I expected. WDYT?
Did you get them all at one time? I see only one at a time. Anyway, if you define them explicitly, IMHO, you should define what and how you want to display.
I don't think that it is a problem: at first there should not be a lot of keywords, so updates should not be frequent. At second: if some new feature was added and user doesn't use that currently - he should now about such possibility, IMHO. For example, imagine that you want to define own highlight for some
I also thought about that and that feature can be added easily - we can just add additional param for highlight config like |
|
@puremourning I just realized that merging is required only for defaults, so I added that without any additional params. Please, take a look |
|
@puremourning Hi! Any updates? |
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @andrejlevkovitch)
README.md line 941 at r6 (raw file):
highlighting. In order to customise the coloring, you should set `g:ycm_semantic_highlight_groups` list. Each item in that list must be dictionary with next fields:
'next fields' -> 'the following keys'
README.md line 942 at r6 (raw file):
`g:ycm_semantic_highlight_groups` list. Each item in that list must be dictionary with next fields: - `filetypes` - list of filetypes that should use this settings for semantic
'this settings' -> 'these settings'
README.md line 942 at r6 (raw file):
properties that are used. If you define a text property named `YCM_HL_<token type>`, then it will be used
we no longer mention these existing highlight group definitions. There are multiple people already using them, so we need to say something about them, even if it is to say that "for compatibility reasons we <....>, but this may be removed in future ..."
README.md line 943 at r6 (raw file):
dictionary with next fields: - `filetypes` - list of filetypes that should use this settings for semantic highlighting. If not defined, then this settings will be used as default for
these settings
README.md line 944 at r6 (raw file):
- `filetypes` - list of filetypes that should use this settings for semantic highlighting. If not defined, then this settings will be used as default for any not defined filetype
"not defined filetype" -> "filetype without explicit configuration"
README.md line 946 at r6 (raw file):
any not defined filetype - `highlight` - dictionary, where key is [token type](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_semanticTokens) and value is highlighting group. If not defined, then semantic highlighting
'if not defined' -> if this means "if this key is not present", then it should say that. These two sentences both talk about disabling things, but it's a little unclear what the difference is.
README.md line 953 at r6 (raw file):
Change this paragraph to read as a full sentence.
In the following example, we set a custom set of highlights for go, disable semantic highlighting for rust, and define default highlighting groups for all other languages:
python/ycm/semantic_highlighting.py line 94 at r6 (raw file):
bufnr = bufnr ) except vim.error: # at YcmRestart we can get error about redefining properties, just ignore them
we should catch specific errors. Usually like if 'E1234' not in error - see elsewhere for how we do this.
I'm also confused why this is now required. What change caused this error to surface?
and why does this not apply to the other branch above.
python/ycm/semantic_highlighting.py line 129 at r6 (raw file):
return # XXX define default settings globally for make it compatible with older
remove XXX
expand comment to explain exactly what this does any why - "compatible with older settings" won't make sense in a few years time.
python/ycm/semantic_highlighting.py line 191 at r6 (raw file):
def _Draw( self ): if self._do_highlight == False:
if I'm reading this correctly, when semantic highlighting is disabled, we still request the tokens, but don't actually draw them? that seems wrong. we should obviously not request the tokens in that case, right?
|
@puremourning Thanks for review! I fixed all the issues, please review that again
That is because here we add property for specific buffer, not globaly |
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @andrejlevkovitch)
python/ycm/semantic_highlighting.py line 148 at r3 (raw file):
Previously, puremourning (Ben Jackson) wrote…
should we break out here? if
filetypesare duplicated, we should provably use the first such list? or should we merge them? I think here we're always using the last such defined set, which means the defaults (in HIGHLIGHT_GROUPS), if set for a filetype would override user settings.
Any comment?
python/ycm/semantic_highlighting.py line 191 at r6 (raw file):
Previously, puremourning (Ben Jackson) wrote…
if I'm reading this correctly, when semantic highlighting is disabled, we still request the tokens, but don't actually draw them? that seems wrong. we should obviously not request the tokens in that case, right?
is this check still even needed?
python/ycm/semantic_highlighting.py line 190 at r7 (raw file):
def _NewRequest( self, request_range ): if self._do_highlight == False: return BaseRequest()
I don't think this is the right way to avoid sending a request.
I guess the reason that we are doing this rather than something more obvious like, just not trying to send a request at all is that it's wrapped up in the base class?
I would much rather we had some explicit, obvious mechanism to say that this object should be dropped right up in the user of this class, otherwise we're doing [all this crap][https://github.com/ycm-core/YouCompleteMe/blob/master/python/ycm/scrolling_range.py#L40-L131) for no reason.
The way I would like to see this is either that we just don't instantiate the semantic_highlighting member, or - and I suspect this is simpler, we add a "_ShouldUseNow()" overload to ScrollingBufferRange which it checks before doing any thing, and have that overloaded here to return self._do_highlight.
|
python/ycm/semantic_highlighting.py line 148 at r3 (raw file):
Sorry, I missed that. I'm not sure - from my perspective it is better to use last one, instead of first one. So, I just don't know what is "correct approach" python/ycm/semantic_highlighting.py line 191 at r6 (raw file):
I think yes, because below it uses "latest response". I'm actually not completely sure if it is possible to be not empty or something like that (I'm not very familiar with the codebase), so I just prefer to keep that for avoid any issues python/ycm/semantic_highlighting.py line 190 at r7 (raw file):
I think so. Base class do nothing in
Yeah, I see your point, but I think it is out of my competence - I don't fill familiar enough with codebase (and python itself) for go deeper, outside of just semantic highlight code file. Sorry |
PR Prelude
Thank you for working on YCM! :)
Please complete these steps and check these boxes (by putting an
xinsidethe brackets) before filing your PR:
rationale for why I haven't.
actually perform all of these steps.
Why this change is necessary and useful
Currently there are no way for set highlighting groups for different file types, so I added such possibility. Also now it is possible to use default vim highlighting groups that defined for specific filetype
This change is