New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix meta.catch.java and meta.throwables.java scopes #144

Merged
merged 2 commits into from Jun 2, 2018

Conversation

Projects
None yet
2 participants
@sadikovi
Member

sadikovi commented May 30, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This PR updates the code for scopes meta.catch.java and meta.throwables.java and fixes scoping of exception class names and/or parameters.

For meta.catch.java we are currently include parameters scope, which is not exactly correct, because parameters are assumed to be storage + variable, but in the case of catch block, you could see something like this Exception1 | Exception2 err, so we would only highlight the second case. I replaced it with the pattern that scopes | as separator and require storage and optional variable name.

For meta.throwables.java we are currently including object-types scope, which is also not 100% correct, because they assume storage followed by variable, but in throws you could see something like this throws Exception1, Exception2. I changed it to just parse storage type.

class MyClass {
  public void set(String str) throws Exception3, Exception4 {
    try {
      // some code
    } catch (Exception1 | Exception2 e) {
      throw new Exception3();
    }
  }
}

Before:
before

After:
after

Alternate Designs

I was considering updating parameters and object-types scopes, but I found that it would be rather massive change, which might cause the regression in other parts. So I went with this fairly straightforward approach.

Benefits

Fixes issue of scoping exceptions in catch and throws.

Possible Drawbacks

None that I found, it does not affect existing scopes.

Applicable Issues

Closes #138

@sadikovi sadikovi changed the title from fix catch and throws clauses to Fix meta.catch.java and meta.throwables.java scopes May 30, 2018

@sadikovi sadikovi requested a review from 50Wliu May 30, 2018

@sadikovi

This comment has been minimized.

Member

sadikovi commented May 30, 2018

@50Wliu could you have a look at this PR? Thanks!

@@ -595,7 +595,16 @@
'include': '#comments'
}
{
'include': '#parameters'
'match': '\\|'
'name': 'punctuation.catch.separator'

This comment has been minimized.

@50Wliu

50Wliu May 31, 2018

Member

.java

'name': 'punctuation.catch.separator'
}
{
'match': '([a-zA-Z$_][\\.a-zA-Z0-9$_]*)\\s*(\\w+)*'

This comment has been minimized.

@50Wliu

50Wliu May 31, 2018

Member

Should it be (\\w+)? instead?

This comment has been minimized.

@sadikovi

sadikovi May 31, 2018

Member

Yes, you are right. I will update the code.

@@ -1218,7 +1227,12 @@
'name': 'meta.throwables.java'
'patterns': [
{
'include': '#object-types'
'match': ','
'name': 'punctuation.throwables.separator'

This comment has been minimized.

@50Wliu

50Wliu May 31, 2018

Member

.java

I also think this can use the standard comma separator scope.

This comment has been minimized.

@sadikovi

sadikovi May 31, 2018

Member

Yes, you are right. Will update.

@sadikovi

This comment has been minimized.

Member

sadikovi commented May 31, 2018

@50Wliu thanks for review! I updated the code, would you mind taking a look again? Thanks.

@50Wliu

50Wliu approved these changes May 31, 2018

@sadikovi sadikovi merged commit b9f1a85 into atom:master Jun 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sadikovi sadikovi deleted the sadikovi:fix-catch-throws-exceptions-scope branch Jun 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment