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

Add more tests to the repo #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Michael-F-Bryan
Copy link

This is a follow-up from the conversation in tree-sitter/tree-sitter#3348.

@Michael-F-Bryan
Copy link
Author

@liamwh would you be able to review this PR?

The only test that doesn't seem to work out of the box is this one:

world foo {
    include wasi:io/my-world-1 with { a as a1, b as b1 };
}

The issue is with the my-world-1.

In my parser, I've defined an identifier like this:

identifier: $ => /%?[a-zA-Z][a-zA-Z0-9-]*/,

Whereas your identifier is more sophisticated and enforces kebab-case where each "word" can't start with a digit:

id: $ => /%?(([a-z][a-z0-9]*|[A-Z][A-Z0-9]*))(-([a-z][a-z0-9]*|[A-Z][A-Z0-9]*))*/,

I checked WIT.md and it doesn't explicitly give a regex for an identifier, but apparently it's meant to match the "label" production from the Component Model, which defines it like this:

label         ::= <fragment>
                | <label> '-' <fragment>
fragment      ::= <word>
                | <acronym>
word          ::= [a-z] [0-9a-z]*
acronym       ::= [A-Z] [0-9A-Z]*

Copy link
Owner

@liamwh liamwh left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for your contribution!

@liamwh
Copy link
Owner

liamwh commented May 13, 2024

Great PR @Michael-F-Bryan! Thanks for this contribution! 👍

Please re-generate the parser.c file by running just test with a version of tree sitter ~0.22.6 and resolve the conflicts with the grammar.js, unfortunately I've added comments that caused conflicts with your PR. Apologies for that! 😅

Regarding the ID regex: I don't see the issue with the id matching showing up in tests, so unless I'm mistaken, it appears we don't need to do anything with that regex at this time?

Cheers,
Liam

@liamwh
Copy link
Owner

liamwh commented Jun 6, 2024

@Michael-F-Bryan Are you still willing to merge your changes to this repo?

@liamwh liamwh force-pushed the main branch 10 times, most recently from 62fc4a7 to 57e31a6 Compare July 4, 2024 16:39
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.

2 participants