Fix Various Syntax Highlighting Issues #631

jfelchner · 2016-09-13T09:06:17Z

I've annotated each commit with what/why of the changes.

This fixes 3 or 4 current outstanding issues.

I've also made some opinionated tweaks to the current highlight linkages. Hopefully you all agree with them. The improvements to the readability of the code I'm currently working on are substantial.

jfelchner · 2016-09-13T09:08:02Z

syntax/javascript.vim

@@ -25,6 +25,7 @@ syntax sync fromstart
 syntax case match

 syntax match   jsNoise          /[:,\;\.]\{1}/
+syntax match   jsSemiEndOfLine  /\;\{1}\(\(\s*\/[\/\*].*\)\?$\)\@=/


FWIW, this also handles the use case of ; followed by comments

Just at a quick glance, I don't like this one at all because it's not contained AND has a look ahead. This will probably destroy performance in a larger file. Also, I am not quite sure the rationale to match end of line semis? Wouldn't it be better to isolate particular semis in contained groups we already have?

amadeus · 2016-09-13T15:51:28Z

Thanks for opening this! It's gonna take me a it to go over this and test it!

amadeus · 2016-09-13T17:23:25Z

syntax/javascript.vim

-syntax keyword jsExceptions     Error EvalError InternalError RangeError ReferenceError StopIteration SyntaxError TypeError URIError
-syntax keyword jsBuiltins       decodeURI decodeURIComponent encodeURI encodeURIComponent eval isFinite isNaN parseFloat parseInt uneval
+syntax keyword jsGlobalObjects      Array Boolean Date Function Iterator Number Object Symbol Map WeakMap Set RegExp String Proxy Promise Buffer ParallelArray ArrayBuffer DataView Float32Array Float64Array Int16Array Int32Array Int8Array Uint16Array Uint32Array Uint8Array Uint8ClampedArray JSON Math console document window Intl Collator DateTimeFormat NumberFormat
+syntax match   jsConstant           "\<[A-Z][A-Za-z]\+\>"


We've had many some similar discussions in the past regarding things how to handle things constants and keywords, and whether to highlight them or not.

My personal opinion on the matter is something like this should NOT be in our syntax file because it's more of a stylistic convention (using capital letters to denote a constants), not a rule of the language.

You could easily add something like this as an after plugin, because an initial capital letter does not a constant make.

Also, this regex could be improved if you use it in an after plugin, since it will break if there are numbers or $ in the name.

How did you even get this diff above?

Oh.... you're looking at the merged diff and not each individual commit.

@amadeus what do you think about a global variable to add in syntax highlighting for constants? Like you do with this, return, etc.

Not sure I follow?

Something like this:

if exists('g:javascript_highlight_upper_camel_as_constants') syntax match jsConstant "\<[A-Z][A-Za-z]\+\>" syntax region jsImportContainer add=jsConstant syntax region jsDestructuringBlock add=jsConstant syntax cluster jsExpression add=jsConstant syntax region jsDestructuringBlock add=jsConstant HiLink jsClassDefinition Constant HiLink jsConstant Constant else HiLink jsClassDefinition Function endif

Hmmm, not sure yet. Let's move that to another issue or PR for further discussion. My initial thought is that it adds clutter where it doesn't need to be, but other maintainers here may have counter opinions and it would be good to isolate that discussion.

jfelchner · 2016-09-13T17:47:23Z

@amadeus all great feedback thanks!

Regarding the end of line semicolons, the look ahead was just for the comments. I'm fine with doing it a different way if you have another suggestion. I'm not sure how you would do it for each individual type of statement and be sure to grab it. But I'll give it a shot. :)

With regards to the constants, no, JavaScript does not force this but have you ever been in a newer codebase (or any codebase) where this was not the convention?

I know with const people often create constants that start with a lowercase letter. That's not what I'm saying. What I'm saying is, have you ever seen a variable that starts with an uppercase letter that you didn't treat like a constant?

So I think that this is something that would be very useful to most developers.

Potentially this could be added as an option like you're doing for concealing certain matches?

jfelchner · 2016-09-13T17:53:38Z

I just realized I missed what you were saying on end of line semicolons. Matching on a particular type of semi wouldn't do what I wanted. :(

The issue is thus:

var foo = 'hello';
var bar = 'goodbye';

Both semis in those lines should be matched. Because they're end of line semis, the have no use to the reader and are noise. They're there only for the compiler (minification, etc)

If I wrote it like this:

var foo = 'hello'; var bar = 'goodbye';

Then only the second semi is noise in this case. The first one acts as a break between statements so that it's easier for the human to parse.

Obviously this is a contrived example.

So if we matched semis on the _type _ of line, it would have matched both in the second example. Which I do not want.

amadeus · 2016-09-13T18:02:03Z

Thing is though, the last semi in a line, to style it differently is still completely a stylistic thing, and even seemingly kind of arbitrary. Take an example like this:

I still think this is a case of you creating an after/syntax/javascript.vim file for JS that applies your stylistic additions and doesn't really belong in a generalized JS syntax file.

amadeus · 2016-09-13T18:03:49Z

On the subject of consts, yes, while I agree it's pretty common, I still personally don't think it belongs since it's a stylistic thing, and starts to get into lexical parsing, which our syntax file isn't really equipped to do.

amadeus · 2016-09-13T18:05:20Z

syntax/javascript.vim

@@ -42,12 +42,12 @@ syntax keyword jsModuleKeywords  contained import
 syntax keyword jsModuleKeywords  contained export skipwhite skipempty nextgroup=jsExportBlock,jsModuleDefault
 syntax keyword jsModuleOperators contained from
 syntax keyword jsModuleOperators contained as
-syntax region  jsModuleGroup     contained matchgroup=jsBraces start=/{/ end=/}/ contains=jsModuleOperators,jsNoise,jsComment
+syntax region  jsModuleGroup     contained matchgroup=jsModuleBraces start=/{/ end=/}/ contains=jsModuleOperators,jsNoise,jsComment


I am fine with this, however it would be good to link it back to jsBraces at the bottom.

It does yes?

jsModuleBraces is not linked to anything right now.

Missed that one! Fixing.

amadeus · 2016-09-13T18:19:43Z

Anyways, overall I like some of these changes. Always nice to get finer granularity with various braces and parens. However I definitely disagree with the stylistic opinionated changes, and therefore I think those should be removed before merging.

You could always remove them to get the good stuff merged in, and we could bike shed in another issue afterwards.

amadeus · 2016-09-13T18:45:05Z

Btw, something like

syntax match   jsConstant           "\<[A-Z][A-Za-z]\+\>" containedin=jsDestructuringBlock
syntax match   jsSemiEndOfLine      /\;\{1}\(\(\s*\/[\/\*].*\)\?$\)\@=/

syntax cluster jsExpression add=jsConstant,jsSemiEndOfLine

hi! def link jsConstant        Constant
hi! def link jsClassDefinition Constant

In after/syntax/javascript.vim should accomplish your stylistic changes.

amadeus · 2016-09-13T18:54:09Z

syntax/javascript.vim

 syntax region  jsExportContainer start=/\<export\> / end="\%(;\|$\)" contains=jsModuleKeywords,jsModuleOperators,jsStorageClass,jsModuleDefault,@jsExpression
-syntax region  jsExportBlock     contained matchgroup=jsBraces start=/{/ end=/}/ contains=jsModuleOperators,jsNoise,jsComment
+syntax region  jsExportBlock     contained matchgroup=jsExportBraces start=/{/ end=/}/ contains=jsModuleOperators,jsNoise,jsComment


Also don't forget to link this to jsBraces as well.

I don't see the changes in this PR yet? Mebe not pushed yet? Or github being slow.

jfelchner · 2016-09-13T19:20:39Z

@amadeus I think I got everything. I'll open another issue with the constant discussion.

amadeus · 2016-09-13T19:25:18Z

syntax/javascript.vim

 syntax match   jsObjectKey         contained /\<[0-9a-zA-Z_$]*\>\(\s*:\)\@=/ contains=jsFunctionKey skipwhite skipempty nextgroup=jsObjectValue
+syntax match   jsObjectColon       contained /:/ skipwhite skipempty


Quick thing - we should link this back to jsNoise for backwards compat.

Many of these are noise, but I don't consider function parenthesis to be so. Rather than creating a non-function parens group, I just created a group for each type and then linked them all to `jsParens` so that everything will behave as it currently does for users who are using `jsParens`.

Similar to the parens from the previous commit, this will allow users to customize the look of various types of braces rather than having to go all-in on non-function braces. ------------------------------------------------------------------------------ Actions: * Fix #589

`this` can be used outside of any other block and thus needs to be highlighted there. ------------------------------------------------------------------------------ Actions: * Fix #610

There were a few issues here: * Class decorators weren't being highlighted since they exist outside of any other group * Decorators which take a parameter didn't have their parameters property highlighted * Decorators which decorated a function within an object weren't being highlighted -------------------------------------------------------------------------------- Actions: * Fixes #590 * Fixes #572

This makes quite a bit more sense to me since decorators are a type of annotation to the function below.

It's right there in the name: "Keyword" and should be highlighted as such.

amadeus · 2016-09-13T19:34:02Z

Outside of the Tag issues I mentioned before, I think this should be good to go. I will test by using it today for a bit, and if everything appears ok, then I'll merge this evening. The only reason I am waiting to test for a couple hours is we have a pretty big merge going out to master soon, and I don't know the timing of when it will get merged, and don't want this to inadvertently hold it back if we find a bug or issue.

jfelchner · 2016-09-13T19:37:07Z

@amadeus you seemed ambivalent on the Tag thing so I left it in. I think it still works. Tag seems like an unused one. Most of the time tagged items are also another item (class name, method name, etc).

All of those groups seem very specific to C IMO. Not all of them apply to other languages. Nor in the same way.

jfelchner · 2016-09-13T19:37:53Z

Up to you. I can add it to my local config.

amadeus · 2016-09-13T19:39:17Z

I definitely thing Structure that we had from before was wrong. So it would be good to fix, it's a quick fix though, let me think about it for a bit.

amadeus · 2016-09-14T18:33:34Z

Doh! Of course yesterday I was heads deep in Objective-C and had no time to test, hopefully today will be ok, or if Dev gets merged into Master, then we can just go ahead and test in dev first, but it's up to @bounceme for when he's ready.

bounceme · 2016-09-14T22:00:38Z

I'm good with this going into the dev -> master merge when you guys are ready. no rush

amadeus · 2016-09-14T22:22:08Z

This is merged, once again, thanks @jfelchner for the PR!

I went ahead and just added the hilinks into develop manually, to get this in.

jfelchner changed the base branch from master to develop September 13, 2016 09:06

jfelchner reviewed Sep 13, 2016
View reviewed changes

amadeus reviewed Sep 13, 2016
View reviewed changes

jfelchner added 9 commits September 13, 2016 14:26

Add highlight group specifically for object colons

4e2e377

Removed 'contained' constraint from 'this' keyword highlight

430b528

`this` can be used outside of any other block and thus needs to be highlighted there. ------------------------------------------------------------------------------ Actions: * Fix #610

Add section comments

aa2fb90

Change highlight link for decorators to 'Tag'

801aad4

This makes quite a bit more sense to me since decorators are a type of annotation to the function below.

Whitespace

6c78b77

Change ClassKeywords to Keywords instead of Define

0cb2e11

It's right there in the name: "Keyword" and should be highlighted as such.

jfelchner mentioned this pull request Sep 13, 2016

Highlighting Upper Camelized Variables as Constants #633

Closed

amadeus merged commit 2bc4663 into pangloss:develop Sep 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Various Syntax Highlighting Issues #631

Fix Various Syntax Highlighting Issues #631

jfelchner commented Sep 13, 2016

jfelchner Sep 13, 2016

amadeus Sep 13, 2016

amadeus commented Sep 13, 2016

amadeus Sep 13, 2016 •

edited

Loading

jfelchner Sep 13, 2016

jfelchner Sep 13, 2016

jfelchner Sep 13, 2016

amadeus Sep 13, 2016

jfelchner Sep 13, 2016

amadeus Sep 13, 2016

jfelchner commented Sep 13, 2016

jfelchner commented Sep 13, 2016 •

edited

Loading

amadeus commented Sep 13, 2016

amadeus commented Sep 13, 2016

amadeus Sep 13, 2016

jfelchner Sep 13, 2016

amadeus Sep 13, 2016

jfelchner Sep 13, 2016

amadeus commented Sep 13, 2016

amadeus commented Sep 13, 2016

amadeus Sep 13, 2016

jfelchner Sep 13, 2016

amadeus Sep 13, 2016 •

edited

Loading

jfelchner commented Sep 13, 2016

amadeus Sep 13, 2016

jfelchner Sep 13, 2016

amadeus commented Sep 13, 2016

jfelchner commented Sep 13, 2016

jfelchner commented Sep 13, 2016

amadeus commented Sep 13, 2016

amadeus commented Sep 14, 2016

bounceme commented Sep 14, 2016

amadeus commented Sep 14, 2016

		syntax match jsObjectKey contained /\<[0-9a-zA-Z_$]\>\(\s:\)\@=/ contains=jsFunctionKey skipwhite skipempty nextgroup=jsObjectValue
		syntax match jsObjectColon contained /:/ skipwhite skipempty

Fix Various Syntax Highlighting Issues #631

Fix Various Syntax Highlighting Issues #631

Conversation

jfelchner commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amadeus commented Sep 13, 2016

amadeus Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfelchner commented Sep 13, 2016

jfelchner commented Sep 13, 2016 • edited Loading

amadeus commented Sep 13, 2016

amadeus commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amadeus commented Sep 13, 2016

amadeus commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amadeus Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

jfelchner commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amadeus commented Sep 13, 2016

jfelchner commented Sep 13, 2016

jfelchner commented Sep 13, 2016

amadeus commented Sep 13, 2016

amadeus commented Sep 14, 2016

bounceme commented Sep 14, 2016

amadeus commented Sep 14, 2016

amadeus Sep 13, 2016 •

edited

Loading

jfelchner commented Sep 13, 2016 •

edited

Loading

amadeus Sep 13, 2016 •

edited

Loading