atom / language-java

Java package for Atom
Other
62 stars 58 forks source link

Fix array initializer highlighting #215

Closed Vigilans closed 4 years ago

Vigilans commented 4 years ago

Description of the Change

Similar to #212, it uses a two-scope pattern to distinguish inner-class block and array-initializer block. it is based on #214 to provide correct end regex for inner-class and array-initializer.

Detailed description

Conside how a syntax scope terminates:

new Class() <end of new expression>
            ^~~~~ terminate here
new Class[] <end of new expression>
            ^~~~~ terminate here
new Class() {} <end of new expression>
               ^~~~~ terminate here
new Class[] {} <end of new expression>
               ^~~~~ terminate here

Previous new-expr grammar handles following ways of termination:

The usage of \s* suffers from new-line and comment-in-between pattern, and this should be relaxed using a two-scope model.

Besides, inner-class and array-initializer do not share same syntax (class-body and code), so they should fall in different scope.

So, this PR:

Alternate Designs

This PR repetitively uses the end regex of new expression:

'end': '(?=;|\\)|\\]|\\.|,|\\?|:|}|\\+|\\-|\\*|\\/(?!\\/|\\*)|%|!|&|\\||\\^|=)'

to determine when a scope terminates. It would be better if we could find a way to reduce its usage.

One possible alternative is: match when inner end token reached (), ] or }), not the outer token that ends it (the complex end regex).

But just like #214, it is hard to tell when ) or ] is met, whether there will be a {} block next to it. I (maybe) tried such silly regexes (cannot remember them correctly):

Benefits

Possible Drawbacks

Applicable Issues

Fix #172 Fix #199 Fix https://github.com/redhat-developer/vscode-java/issues/728

Vigilans commented 4 years ago

Demo

Before

image

After

image

sadikovi commented 4 years ago

Can you describe what alternatives were considered?

Vigilans commented 4 years ago

Hi @sadikovi, I updated how the modification of this PR is formed, and the alternative I tried. Some other alternatives led to similar results.

sadikovi commented 4 years ago

Workaround is removing whitespace between ] {.

Vigilans commented 4 years ago

@sadikovi

Workaround is removing whitespace between ] {.

It reminds me of another alternative I tried...

(?<!\\])\\s*({) uses negate look back pattern, so although it will not match ]{, it will match <space>{ in ]<space>{.

172 proposed another regex, using look back without negation: (?<=\\))\\s*({), but it will cause the tests to fail since the \s* prevents inner-class's left bracket { from being placed in new line.

So either array-init or inner-class with suffer from new line or comment in between as long as \s* is used, this is how another scope is introduced.

Eskibear commented 4 years ago

Good to know that someone is fixing it now! Is this going to be merged?

sadikovi commented 4 years ago

@Eskibear hopefully :)

@Vigilans Can you rebase? Also, if I remember correctly it fixes only part of the problem with array initialisers. Could you clarify? Thanks!

Vigilans commented 4 years ago

Hi, I've rebased my branch and removed a redundant commit.

if I remember correctly it fixes only part of the problem with array initialisers.

This PR fixes full of the problem with the choice of end regex '(?=;|\\)|\\]|\\.|,|\\?|:|}|\\+|\\-|\\*|\\/(?!\\/|\\*)|%|!|&|\\||\\^|=)'. What falls short are the alternate designs I mentioned, in compare with the repetitively used end regex they fixes only part of the problem.

sadikovi commented 4 years ago

Thanks! Merging to master.

Eskibear commented 4 years ago

Great appreciation! So when is it supposed to be shipped?

sadikovi commented 4 years ago

I will try preparing release this weekend if I have time.