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

Add support for contentRegex #109

Merged
merged 2 commits into from
Apr 19, 2019
Merged

Add support for contentRegex #109

merged 2 commits into from
Apr 19, 2019

Conversation

Aerijo
Copy link
Contributor

@Aerijo Aerijo commented Sep 17, 2018

Tree-sitter grammars appear to have a new property contentRegex. Support for this is already in Atom, all that's required for TextMate to benefit from it is to add in when constructing the grammar.

In particular, this will help differentiate similar grammars such as LaTeX & LaTeX Beamer. In this case, both share the same file extension, and both can potentially not match the first line regex. It is difficult / impossible to influence the score in a way that works in all cases.

With the change, the solution is just add contentRegex to Beamer. If it matches, it gets a edge over LaTeX. If it fails, it gets a penalty and LaTeX consistently gets applied.

Alternatively, Atom could be changed to give a penalty when the firstLineMatch fails, but this may have unwanted consequences.

@lee-dohm
Copy link
Contributor

@maxbrunsfeld Would you take a look here since you're the expert on the contextRegex thing?

@maxbrunsfeld
Copy link
Contributor

With Tree-sitter grammars, the contentRegex is just a javascript regex, not an oniguruma regex. I don't have super strong opinions on how it should behave with the TextMate grammars, but as written, this probably won't quite work because oniguruma regexes have a different API than regular regexes (.test vs .testSync).

@Aerijo
Copy link
Contributor Author

Aerijo commented Sep 25, 2018

@maxbrunsfeld Would it work if Atom pulled it out directly, and either stored it or added it to the constructed grammar?

@lee-dohm
Copy link
Contributor

I'm not sure that this is going to translate correctly. Perhaps we should hold off on this for now?

@lee-dohm
Copy link
Contributor

@maxbrunsfeld Would you state what changes are necessary here?

@maxbrunsfeld
Copy link
Contributor

Options I see:

  1. Merge this PR as-is. This will require some changes in Atom when we upgrade first-mate in Atom, because Atom expects contentRegex to be a RegExp (not an OnigRegExp). Specifically, Atom would need to have different logic around contentRegex depending on whether the grammar is a TreeSitterGrammar or a FirstMate.Grammar.

  2. Change this code to instantiate a RegExp instead of an OnigRegExp. This would be simpler on Atom's side, but is maybe a bit inconsistent, because all of the other regexes in first-mate are oniguruma regexes?

@Aerijo
Copy link
Contributor Author

Aerijo commented Nov 27, 2018

@maxbrunsfeld I submitted a PR to Atom that will handle it as an Oniguruma regex. At first I was happily surprised it had a method named test, but unfortunately it was async so changes were required.

As noted there, it depends on this PR to add the contentRegex property to the grammar, and would therefore need to have the first-mate version bumped.

@ashthespy
Copy link

@Aerijo @maxbrunsfeld Any thing preventing this from being merged?

@Aerijo
Copy link
Contributor Author

Aerijo commented Feb 4, 2019

@ashthespy Don't think so, besides making sure the changes are coordinated and the tests pass on the other PR.

@nathansobo nathansobo assigned nathansobo and unassigned nathansobo Apr 10, 2019
@nathansobo nathansobo self-assigned this Apr 19, 2019
@nathansobo
Copy link
Contributor

Thanks very much!

@nathansobo nathansobo merged commit e25b110 into atom:master Apr 19, 2019
@nathansobo
Copy link
Contributor

Published as first-mate@7.2.0.

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.

5 participants