Skip to content

Conversation

@DanTup
Copy link
Contributor

@DanTup DanTup commented Jul 27, 2022

@bkonyi This updates to the latest grammar, which includes a few fixes and better highlighting for interpolated strings.

Before:

Screenshot 2022-07-27 at 16 56 09

After:

Screenshot 2022-07-27 at 16 47 27

However, the parser did not support having patterns nested in captures so I also had to make some changes there. I extracted a PatternMatcher that matches anything that has a set of patterns (and added this to _Matcher.parse()). and now specifically look for nested patterns in captures.

With these changes, I get the same output when running some tests (see dart-lang/dart-syntax-highlight#28) as a NodeJS tool I was using to do the same. I have found some issues with the grammar (such has handling of nested type args), but I'd like to get and get some tests for the grammar (via dart-lang/dart-syntax-highlight#28) before I start trying to fix that, to ensure I don't regress anything while trying to fix (since I don't expect the fix to be trivial).

Please review carefully, I'm a parser noob :-)

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@DanTup DanTup requested a review from bkonyi July 27, 2022 15:54
Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

It's been awhile since I've looked at the parser code, but I've looked through this a couple of times and I don't see anything obviously wrong, so LGTM.

@DanTup
Copy link
Contributor Author

DanTup commented Jul 28, 2022

I've pushed updates to the span parser tests for latest changes, although I've found a few other differences with the NPM tool I ran that I haven't tracked down yet, so I'm going to try and get to the bottom of them before landing this (in case they're issues introduced by this change).

@kenzieschmoll
Copy link
Member

Is this PR still blocked or is it ready to land?

@DanTup
Copy link
Contributor Author

DanTup commented Aug 8, 2022

This isn't good to go yet. There are a few other issues with the parser I want to address before trying to upgrade to the latest grammar (which includes these additional things we don't handle). I'll close this and file a new PR when I'm ready to upgrade.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants