PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside the brackets) before filing your PR:

• [x] I have read and understood YCM's [CONTRIBUTING][cont] document.
• [x] I have read and understood YCM's [CODE_OF_CONDUCT][code] document.
• [ ] I have included tests for the changes in my PR. If not, I have included a rationale for why I haven't.
• [x] I understand my PR may be closed if it becomes obvious I didn'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 

Jul 22 '23 12:07 andrejlevkovitch

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.

Jul 22 '23 15:07 puremourning

Why would a user want to do this in general and specifically?

Sorry, I thought that it is obvious. So:

1. I use different highlighting for different languages, like C/C++, rust and go. They don't differ much, but some highlighting can be a bit different
2. semantic of languages also can be a bit different, so some highlighting will not work properly. Just as example: I use special highlighting for methods in C++, that is different from functions, but for Go, that doesn't have a lot of sense, because methods will be highlighted properly only for declaration, but call of function/method is always a "function". So, for be consistent I use same highlight for functions and methods in Go
3. Some languages almost doesn't require semantic highlighting due to language syntax. Like Rust. Only one thing that I use for rust is highlighting for types. So, for Rust I disable everything, except types

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

is it a breaking change?

I don't think so

does it make existing setups no longer work as they did before?

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

Jul 22 '23 18:07 andrejlevkovitch

If you defined some special highlighting previously, for example for functions, then it will not used anymore. Instead it will use default highlight.

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.

Jul 22 '23 19:07 puremourning

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

Jul 22 '23 21:07 andrejlevkovitch

Great thanks. I'll try to give this a spin later.

Jul 23 '23 07:07 puremourning

Codecov Report

Merging #4167 (96698c1) into master (71166ea) will decrease coverage by 11.10%. The diff coverage is 53.62%.

:exclamation: Current head 96698c1 differs from pull request most recent head cfb7119. Consider uploading reports for the commit cfb7119 to get more accurate results

@@             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     

Jul 23 '23 07:07 codecov[bot]

@puremourning any updates on that?

Jul 26 '23 15:07 andrejlevkovitch

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 !

Jul 26 '23 22:07 puremourning

is there a way to simplify at all?

IDK

There are 2 choices, pure python unit test in python/ycm/test, or preferably full-vim integration test in tests/

Sorry, but I am not python developer and I don't know the codebase well, so, probably, I can not do that

I think this will actually change the value of g:ycm_semantic_highlight_groups. Intentional?

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

It's not guaranteed that the filetypes of bufnr are the same as the "current" filetypes. We should get the filetypes for bufnr

Yeah, that makes sense

should we break out here? if filetypes are duplicated, we should provably use the first such list? or should we merge them?

Yeah, that would be better, I wanted to do that at first, but there are no goto statement in python. So, instead it requires additional variable and additional check. Also, if there are duplicated settings - that is probably "undefined". Not sure what we should with that. And should we? So, for simplification, I left it as it is now

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.

yeah, that is right, but I don't think that we should set defaults per files - lets leave it for user

Jul 29 '23 14:07 andrejlevkovitch

made some fixes, please take a look

Jul 29 '23 14:07 andrejlevkovitch

@puremourning were you able to check the latest changes?

Jul 31 '23 16:07 andrejlevkovitch

@puremourning Hi! Please take a look

Aug 14 '23 08:08 andrejlevkovitch

The result was that semantic highlighting no longer works and I get ...

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.

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 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 rust, so you just copied c highlight and changed some colors. But rust has some tokens that c doesn't have and if we don't display that message - user never knew that highlight doesn't cover everything

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 also thought about that and that feature can be added easily - we can just add additional param for highlight config like use_default_highlight_for_not_defined_tokens - or something like that. So, if that is True, then we just merge default highlight with user defined. Otherwise we don't. I'm not sure what should be default value. Please let me know what do you think and what name for that field will be better

Aug 19 '23 07:08 andrejlevkovitch

@puremourning I just realized that merging is required only for defaults, so I added that without any additional params. Please, take a look

Aug 24 '23 11:08 andrejlevkovitch

@puremourning Hi! Any updates?

Oct 09 '23 09:10 andrejlevkovitch

@puremourning Thanks for review! I fixed all the issues, please review that again

I'm also confused why this is now required. What change caused this error to surface?

That is because here we add property for specific buffer, not globaly

Dec 21 '23 19:12 andrejlevkovitch

python/ycm/semantic_highlighting.py line 148 at r3 (raw file):

Any comment?

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):

is this check still even needed?

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 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 think so. Base class do nothing in Start method, it always return true in Done and return empty map in Response. So, I'm pretty sure that it doesn't send any requests

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.

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

Jan 03 '24 10:01 andrejlevkovitch