Skip to content
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

Support contentRegex for TextMate grammar #18499

Merged
merged 9 commits into from Apr 19, 2019

Conversation

Projects
None yet
2 participants
@Aerijo
Copy link
Member

commented Nov 27, 2018

Requirements for Adding, Changing, or Removing a Feature

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must contribute a change that has been endorsed by the maintainer team. See details in the template below.
  • The pull request must update the test suite to exercise the updated functionality. For guidance, please see https://flight-manual.atom.io/hacking-atom/sections/writing-specs/.
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see https://github.com/atom/atom/tree/master/CONTRIBUTING.md#pull-requests.

Issue or RFC Endorsed by Atom's Maintainers

atom/first-mate#109

Description of the Change

NOTE: Requires update on first-mate dependency to make the contentRegex property be picked up in the first place.

Adds support for the contentRegex property existing on TextMate grammars. Treats it as an Oniguruma regex, so that the TextMate grammar is consistent with itself.

Alternate Designs

Could make the contentRegex a regular JS regex, but then it would be the only regex like that for a TextMate grammar.

Possible Drawbacks

Currently it looks at the whole file, which is potentially dangerous for performace. The use case in the linked issue would be fine with 20 lines or less. Declaring how many lines it wants would require more design choices though.

Verification Process

Manually

Release Notes

  • Add support for contentRegex on TextMate grammars

@nathansobo nathansobo self-assigned this Apr 19, 2019

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

I added some basic test coverage for the added logic. Waiting on a green build and then I will merge. Thanks for contributing this!

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Argh. Got bit by a flaky build failure that I'm actively fighting on another thread. Rebuilding.

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Re-triggered the build manually and it passed. Not sure why that is not indicated on the PR.

@nathansobo nathansobo merged commit bca3e5b into atom:master Apr 19, 2019

1 of 2 checks passed

Atom Pull Requests #20190419.13 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@nathansobo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Thanks again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.