Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Fix catch parameter highlighting#220

Merged
sadikovi merged 3 commits intoatom:masterfrom
Vigilans:vigilans/catch-parameter
Dec 8, 2019
Merged

Fix catch parameter highlighting#220
sadikovi merged 3 commits intoatom:masterfrom
Vigilans:vigilans/catch-parameter

Conversation

@Vigilans
Copy link
Contributor

@Vigilans Vigilans commented Dec 7, 2019

Description of the Change

Uses a two-scope begin/end model to correctly highlight catch parameter.

It wraps Exception ... | or Exception ... ) as a scope, allowing parameter to only sit in this scope, to resolve the parsing sequence issue (first exception type, then parameter identifier).

Alternate Designs

A more complex two-scope design:

  • scope 1: (...Exception
  • scope 2: |...Exception
  • scope 3: parameter...)

Problem: cannot correctly highlight when there's a storage-modifer (like final)

Recursive design:

catch-parameters:
  begin: /(?<=\()|(\|)/
  beginCapture.0.name: 'catch.separator'
  end: /(?=\))/
  patterns:
    - include: storage-modifier
    - include: exception-type
    - include: catch-parameters # recursive
    - include: paramter-identifier

It makes use of the fact that catch (ex1 | ex2 | ex3 param) is in terms of structure the same as catch (ex1 (ex2 (ex3 param) (every time a | is met, the remaining part is actually a new catch-parameter pattern)

Problem: still cannot elegantly handle the base case: catch (final ex3 param).

Benefits

Can correctly highlight catch parameter when it is placed in new line or with a comment in between.

Possible Drawbacks

Don't see any afak.

Applicable Issues

Fix #219

Copy link
Contributor

@sadikovi sadikovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests don't cover all of the code changes.

@Vigilans Vigilans requested a review from sadikovi December 7, 2019 17:10
@Vigilans
Copy link
Contributor Author

Vigilans commented Dec 7, 2019

I enriched the two testcases by inserting comments into possbile locations related with my changes.

Copy link
Contributor

@sadikovi sadikovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for updating the test!

@sadikovi
Copy link
Contributor

sadikovi commented Dec 8, 2019

Merging to master, thanks!

@sadikovi sadikovi merged commit 0facf7c into atom:master Dec 8, 2019
@Vigilans Vigilans deleted the vigilans/catch-parameter branch December 9, 2019 04:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Catch parameter not correctly highlighted when declared in new line or with a comment in between

3 participants