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

Attempt at regex for validation #2148

Closed
wants to merge 2 commits into from

Conversation

pokey
Copy link
Member

@pokey pokey commented Dec 17, 2023

Certainly a lot less code. Also allows things like statement.startOf, which should be allowed I think. We have zero-width captures, eg for name of start tag with react fragments

Checklist

@pokey pokey changed the title Pokey/attempt-at-regex-for-validation Attempt at regex for validation Dec 17, 2023
@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Dec 17, 2023

I don't think you should able to do scope.leading.start I think the notion that some relationship captures are position and some are ranges is important.

@pokey
Copy link
Member Author

pokey commented Dec 17, 2023

but if you do scope.leading, then it's actually a range corresponding to the node you've tagged

@pokey
Copy link
Member Author

pokey commented Dec 17, 2023

added a couple more tests

@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Dec 17, 2023

but if you do scope.leading, then it's actually a range corresponding to the node you've tagged

We only use start. We don't care about the range. Then supporting start and end is misleading.

@pokey
Copy link
Member Author

pokey commented Dec 18, 2023

Ok fine; I added a few more tests on this branch. If you can pass all my tests then your code is fine 😊

The regex thing was just a quick 15-min experiment to see if it was clearer, because this task feels very much like what regexes were designed for, but after implementing it I'm not sure I'm convinced the regex approach is any clearer 😅

I think the clearest would be a parser, but that is most definitely overkill 😄

@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Dec 19, 2023

I started with a regex, but I thought it became to hard to read and reason about that I went to the whitelist approach which is much simpler. You can even print the whitelist if you want :D

A parser would be nice because then we can complain about using wild card with no "named scope" and similar problems. For now I think it might be overkill.

I don't think we should support @_foo. That kind of capture was only used in a single language go and wasn't even needed. For now I think we should just stick with dummy.

@pokey
Copy link
Member Author

pokey commented Dec 19, 2023

I started with a regex, but I thought it became to hard to read and reason about that I went to the whitelist approach which is much simpler. You can even print the whitelist if you want :D

Makes sense 😄

A parser would be nice because then we can complain about using wild card with no "named scope" and similar problems. For now I think it might be overkill.

Sorry what are the 'no "named scope" and similar problems'?

I don't think we should support @_foo. That kind of capture was only used in a single language go and wasn't even needed. For now I think we should just stick with dummy.

Oh ok I had thought we were moving in the direction of the _ prefix after a discussion on a PR, but I guess maybe you weren't involved in that one. The reason to prefer _ prefix is that it gives us more flexibility to be clearer / more verbose; "dummy" tells you nothing about what / why you're tagging something. It also feels a bit special / surprising that just that one special word works. Happy to discuss if you disagree tho

@AndreasArvidsson
Copy link
Member

A parser would be nice because then we can complain about using wild card with no "named scope" and similar problems. For now I think it might be overkill.

Sorry what are the 'no "named scope" and similar problems'?

A named scope capture like @statement. You can't just have a wildcard without a named scope in the same pattern.

I don't think we should support @_foo. That kind of capture was only used in a single language go and wasn't even needed. For now I think we should just stick with dummy.

Oh ok I had thought we were moving in the direction of the _ prefix after a discussion on a PR, but I guess maybe you weren't involved in that one. The reason to prefer _ prefix is that it gives us more flexibility to be clearer / more verbose; "dummy" tells you nothing about what / why you're tagging something. It also feels a bit special / surprising that just that one special word works. Happy to discuss if you disagree tho

I can't remember a discussion like that, but I can see the logic behind it. So we should allow any capture that starts with a underscore but they can't have any relationships or positions?

@pokey pokey mentioned this pull request Jan 23, 2024
1 task
@pokey
Copy link
Member Author

pokey commented Feb 9, 2024

Incorporated changes into #2147

@pokey pokey closed this Feb 9, 2024
@pokey pokey deleted the pokey/attempt-at-regex-for-validation branch February 9, 2024 16:23
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