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

Add Java 9/10 keywords #143

Merged
merged 1 commit into from Jun 2, 2018

Conversation

Projects
None yet
3 participants
@snjeza
Contributor

snjeza commented May 29, 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 adds reserved keywords open,module,requires,transitive,exports,opens,to,uses,provides and with, so they are highlighted as standard keywords. I assigned scope keyword.modules.java for them, because they are used in modules-info.java.
See §3.9 in http://cr.openjdk.java.net/~mr/jigsaw/spec/java-se-9-jls-diffs.pdf

Before:
527a

After:
527

Alternate Designs

None were considered, because it is fairly simple change.

Benefits

Java 9/10 keywords are now highlighted the same way as standard keywords.

Possible Drawbacks

None.

Applicable Issues

#142

@sadikovi

This comment has been minimized.

Member

sadikovi commented May 30, 2018

Thank you for the pull request, I will have a look shortly.
Could you fill out pull request template (required) that describes the change and tests (or add a PR description)? Thanks.

@sadikovi

This comment has been minimized.

Member

sadikovi commented May 31, 2018

Thanks for updating the description.

After thinking about it a bit more, I think we might want to take a slightly different approach. These keywords look specific to a module. For example, I was thinking if it would be better to have module scope that resembles class scope, since they are very similar, and have all those keywords within the module scope. This approach would not break existing highlighting in the code that uses them as identifiers - the keywords are only "keywords" within a module, not in other code.

Here is an example (one of the ways to do it):

Include our scope to global patterns, since it has its own file, sort of:

{
  'include': '#module'
}

Then we would create an initial draft of module, which is already pretty good. This allows us to extend it later with other rules, e.g. requires XYZ; only on one line, etc.

'module':
  # a uniquely named, reusable group of related packages, as well as resources (such as images
  # and XML files).
  'begin': '((open)\\s)?(module)\\s+(\\w+)'
  'end': '}'
  'beginCaptures':
    '1':
      'name': 'storage.modifier.java'
    '3':
      'name': 'storage.modifier.java'
    '4':
      'name': 'entity.name.type.module.java'
  'endCaptures':
    '0':
      'name': 'punctuation.section.module.end.bracket.curly.java'
  'name': 'meta.module.java'
  'patterns': [
    {
      'begin': '{'
      'beginCaptures':
        '0':
          'name': 'punctuation.section.module.begin.bracket.curly.java'
      'end': '(?=})'
      'contentName': 'meta.module.body.java'
      'patterns': [
        # TODO: Write more rules for module grammar
        {
          'match': '\\b(requires|transitive|exports|opens|to|uses|provides|with)\\b'
          'name': 'keyword.module.java'
        }
      ]
    }
  ]

Below is the diff for this, you can use it if you want, or you can write your own (it is a fun exercise, IMHO): https://gist.github.com/sadikovi/d1a400ef3020be2e07999f20d47fbe74

This is the result of the patch:
shot

@sadikovi

This comment has been minimized.

Member

sadikovi commented May 31, 2018

@50Wliu could you let us what you think about the module keywords? Thanks.

@thomasjo thomasjo requested a review from 50Wliu May 31, 2018

@snjeza

This comment has been minimized.

Contributor

snjeza commented May 31, 2018

@sadikovi I have updated the PR.

@sadikovi

This comment has been minimized.

Member

sadikovi commented May 31, 2018

Thanks! This change will allow anyone to easily update/improve module later without breaking other code. Could you add a unit test that verifies the change? You can find the examples in spec/java-spec.coffee.

@50Wliu

Looks good pending specs.

@sadikovi

This comment has been minimized.

Member

sadikovi commented Jun 1, 2018

@snjeza Looks good! Could you also check Provider, { and } scopes as well in the added test?

@sadikovi

This comment has been minimized.

Member

sadikovi commented Jun 1, 2018

Looks good. @50Wliu do you have any more comments? If not, I will merge it!

}
'''
expect(lines[0][0]).toEqual value: 'module', scopes: [ 'source.java', 'meta.module.java', 'storage.modifier.java' ]

This comment has been minimized.

@50Wliu

50Wliu Jun 1, 2018

Member

For consistency, there shouldn't be space padding on the insides of the brackets.

Add Java 9/10 keywords
Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

@sadikovi sadikovi merged commit faafae0 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

This comment has been minimized.

Member

sadikovi commented Jun 2, 2018

@snjeza Thanks the PR! @50Wliu Thanks for the review!

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