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

Implement more string token escaping in the parser translator #3361

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Earlopain
Copy link
Contributor

This leaves \c and \M escaping but I don't understand how these should even work yet. Maybe later, they seem uncommon enough.

It seems a bit redundant to implement this again when prism already knows how to do all this, and will probably do a better job as well. Maybe it's ok? The implementation is not too complicated I think. Open to hear different approaches.

@Earlopain Earlopain force-pushed the parser-translator-complex-escaping branch from 4739740 to b75d94c Compare January 3, 2025 22:25
This leaves `\c` and `\M` escaping but I don't understand how these should even work yet. Maybe later.
@Earlopain Earlopain force-pushed the parser-translator-complex-escaping branch from b75d94c to 3b2eed4 Compare January 3, 2025 22:27
@kddnewton
Copy link
Collaborator

I could be convinced to go this way, but definitely would prefer to share the logic instead of reimplementing. It would involve exposing the unescaping logic from prism.c as Prism.* functions that we could reuse. Maybe we can save that for a later refactoring.

@Earlopain
Copy link
Contributor Author

Yes, I totally agree that that should be shared, escape_read looks like it is doing the bulk of the heavy lifting and I certainly don't want to replicate all of that. But I don't think I'm able to do so myself, I never did much C and it looks very interconnected with doing actual parsing (which makes sense).

For now I would say this is good enough but I'd also understand if you don't want to bother with this. I'll open an issue for such an API if that is OK with you.

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