Skip to content

Update tokenize to strip permissions correctly #71

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

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Conversation

hammo92
Copy link
Collaborator

@hammo92 hammo92 commented Jan 30, 2025

The current tokeniser doesn't correctly remove the permission clause from the definition, move the permissions check to the top of the function and use regex to correctly remove it after it has been matched. Fixes #70

@hammo92
Copy link
Collaborator Author

hammo92 commented Jan 30, 2025

Need to fix the biome checks

@hammo92
Copy link
Collaborator Author

hammo92 commented Jan 30, 2025

So instead of our previous regex approach I think a more robust method would be to use the field clauses as limiters, as we know that they serve as separation points for the clauses, this way we're less likely to have clauses ending up merged.

@sebastianwessel
Copy link
Owner

we should add some tests, as it becomes complex @hammo92 ?

@hammo92
Copy link
Collaborator Author

hammo92 commented Feb 3, 2025

sure can't hurt to add some additional tests; I'll put some together

@hammo92
Copy link
Collaborator Author

hammo92 commented Feb 5, 2025

We have pretty comprehensive tests already on the tokeniser, I think fine to merge to fix the current issues. I will be adding a couple of new updates to handle some issues with array schema generation and for literals so will check again then.

@hammo92 hammo92 merged commit b5a96fb into main Feb 5, 2025
2 checks passed
@sebastianwessel sebastianwessel deleted the tokeniser_fix branch February 5, 2025 19:57
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.

ASSERT breaks the code generation
2 participants