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 accessor keyword in class properties #353

Merged
merged 9 commits into from
Dec 26, 2022

Conversation

jogibear9988
Copy link
Contributor

No description provided.

@lahma
Copy link
Collaborator

lahma commented Dec 26, 2022

I've updated the test suite so if you rebase and remove lines from allow-list.txt having grammar-field-accessor in their name (total of 4 lines) it should cover new cases that tests want to parse .

@jogibear9988
Copy link
Contributor Author

will do, at first i need to fix broken tests

@jogibear9988
Copy link
Contributor Author

@lahma please review.

I also needed to add some tests to the allow list, cause the generated _characterData seems not to be correct.

But I did not understand the regex to fit it

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, the allow list shows that we are moving to the right direction. I'm willing to merge if ready, we probably need more cleanup to the class parsing logic later like you've discovered that it's a bit complex.

So should we merge?

@jogibear9988
Copy link
Contributor Author

for me it's ready, i'm done atm

@lahma lahma changed the title work on accessor keyword in class properties Support accessor keyword in class properties Dec 26, 2022
@lahma lahma merged commit 29ccbb2 into sebastienros:main Dec 26, 2022
@lahma
Copy link
Collaborator

lahma commented Dec 26, 2022

Thanks for going through the spec and implementation, always appreciated!

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