search

StanAngeloff / php.vim

An up-to-date Vim syntax for PHP (7.x supported)
477 stars 69 forks source link

Incorrectly associated group #81

Closed dsifford closed 6 years ago

dsifford commented 6 years ago

Hi there.

Great work on this syntax! Super robust!

One small little gripe I have that I'm hoping you might consider is the following:

https://github.com/StanAngeloff/php.vim/blob/9f2d8fb0030684af53f30a6e532289948e83facc/syntax/php.vim#L525-L526

Shouldn't this group instead be phpKeyword? You literally use the word keyword in your comment describing it.

Thanks for considering

StanAngeloff commented 6 years ago

Thanks for poking around in the syntax file! I'm merely looking after the great work done by others as can be seen in the header comment.

Unfortunately making this change may break users syntax colouring or overrides.

dsifford commented 6 years ago

@StanAngeloff I'm not sure what you mean by "the change might break users syntax coloring overrides".

To clarify: My claim is that the group that exists right now is currently breaking everybody's syntax coloring. This is a clear violation of syntax as the words public, private, protected, etc are language keywords and should be treated as language keywords.

So the notion that we should keep as-is because people have put guards in place already to cover this break is not valid. It should most definitely be fixed here.

I just tried in a clone of this repo and making the changes suggested in my PR fix this completely and align the syntax similarly to all other vim syntaxes that use these types of keywords.

dsifford commented 6 years ago

This is a big deal particularly because this repo is the syntax used by vim-polyglot so there's not a super easy way to fix this without convincing the polyglot maintainers to choose a different fork.

StanAngeloff commented 6 years ago

But they are keywords, i.e., syn keyword? The group name is arbitrary.

StanAngeloff commented 6 years ago

Perhaps the confusion is coming from the fact that phpType is linked to Type and not Keyword?

dsifford commented 6 years ago

The syntax coloring from themes stems from the group it's allocated to... The keyword parameter doesn't have any meaning except to vim it states that the list of words should be treated as a list of words, and not regexps.

So, in other words: To the outside world, vim treats those words as "types", and not keywords.

Does that make sense?

dsifford commented 6 years ago

Put simply: Right now, those words have the syntactical scope of "type" and not "keyword"

StanAngeloff commented 6 years ago

As above, perhaps the confusion is coming from the fact that phpType is linked to Type and not Keyword. I'm not inclined to make the change as it will break existing syntax highlighting.

The README has a section on how to add overrides, adding this should suffice:

" Put this function at the very end of your vimrc file.

function! PhpSyntaxOverride()
  hi! link phpType Keyword
endfunction

augroup phpSyntaxOverride
  autocmd!
  autocmd FileType php call PhpSyntaxOverride()
augroup END
dsifford commented 6 years ago

I understand where you're coming from, but the syntax is already broken. That's the point.

Users 100% should not have to put in "overrides" for bugs. This is something that needs to be handed here.

If you are adamant about not fixing this, I'll be opening an RFC in vim-polyglot to move off of this fork.

This is absolutely stunning to me.

Edit::

perhaps the confusion is coming from the fact that phpType is linked to Type and not Keyword

Why would phpType, be linked to keyword? phpType is a type. So that's correctly linked already. The singular issue is that the keywords class abstract extends interface implements static final var public private protected const trait are also linked to type. That's it. Keywords just need to be keywords.

StanAngeloff commented 6 years ago

Thank you for your input. If this is a deal-breaker for you or vim-polyglot, please feel free to fork and maintain a copy with your updates :+1:

dsifford commented 6 years ago

Will do. Thanks

sheerun commented 6 years ago

Could the solution be to duplicate this line instead of removing it?

StanAngeloff commented 6 years ago

Could the solution be to duplicate this line instead of removing it?

A couple of the keywords are later on duplicated indeed. Most are not and are highlighted in a group phpType. As I pointed out above, phpType has been the case since 2010. Newer versions of Vim ship with a different syntax for PHP that doesn't suffer from this perceived bug.

I'm all for preserving backwards compatibility. This is why I'm not inclined to update something which has been the norm for 7+ years and people depend on it (I don't personally).

sheerun commented 6 years ago

Totally understand. Usually it's the opposite - plugins break compatibility while core vim syntaxes are more stable. I think it would be reasonable to configure vim-polyglot to ship what newest vim is shipping then.