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 stateful parser #8

Closed
wants to merge 2 commits into from
Closed

Conversation

emcfarlane
Copy link
Collaborator

This is an alternative stateful parser implementation. It main method is parsePathTemplate which takes the route config and returns pathSegments and pathVariables. This removes the logic to recompute the variable capture in the router whilst still keeping the parser clean.

@emcfarlane emcfarlane requested a review from jhump August 8, 2023 11:51
@emcfarlane emcfarlane self-assigned this Aug 8, 2023
Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

This lexer seems overly complicated. You basically have repeated the grammar in both the lexing phase and the parse phase, which leads to code bloat. I think it's overkill since lexical analysis for this language does not need that kind of context. The only place that benefits is distinguishing path literals from identifiers, but that's easy enough to handle in the parser by accepting a sequence of tokens (identifiers and non-identifiers) as a single literal.

I also notice that your implementation accepts wildcards in the middle of path elements, which seems highly error-prone for users, since it means a literal asterisk and is not interpreted as a wildcard. Is that lifted from grpc-gateway?

@emcfarlane
Copy link
Collaborator Author

@jhump I don't understand the complexity argument. It's around the same loc and helps reduce the logic needed in the parser.

It doesn't accept wildcards in the middle of path elements.

@jhump
Copy link
Member

jhump commented Aug 8, 2023

It's around the same loc and helps reduce the logic needed in the parser.

LOC is not complexity (simpler code can be more verbose). This doesn't reduce the logic: it still needs to enforce the grammar rules, but now it also tracks more state in order to produce a flattened model. The added state and logic is what I mean by increasing complexity -- it is "complecting" additional concerns into the parse operation. Perhaps by "simplify" you mean removing the recursive quality?

It doesn't accept wildcards in the middle of path elements.

My bad, I misinterpreted this line (which I see you've now removed).

@emcfarlane
Copy link
Collaborator Author

Closing, might be worth revisiting as a lexerless parser but not needed.

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