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

derive: refactor parser #772

Merged
merged 2 commits into from
Jan 30, 2023
Merged

derive: refactor parser #772

merged 2 commits into from
Jan 30, 2023

Conversation

Kijewski
Copy link
Collaborator

parser.rs was a single file containing almost 2000 lines.

This PR split the file into multiple, smaller files. Expr, Node, and Target each get an own file. Each struct gets a parse() method that return Result<Self>, and every other detail is private to the file.

This PR should make this essential part of Askama more easy to understand, and make future modifications easier.

@djc
Copy link
Collaborator

djc commented Jan 30, 2023

Good stuff! Some thoughts on the new structure:

  • I'd like to drop the parse_ prefix on modules within parser, since it seems a bit stuttery to have both.
  • I think we stuff in parse_common could just move into parser/mod.rs directly.
  • I'd prefer to move the contents of parse_target into parse_node.
  • I'd then also like to move the Expr definition into the expr module and the Node and Target definitions into node, so that we no longer need a separate ast module.

`parser.rs` was a single file containing almost 2000 lines.

This PR split the file into multiple, smaller files. `Expr`, `Node`, and
`Target` each get an own file. Each struct gets a `parse()` method that
return `Result<Self>`, and every other detail is private to the file.

This PR should make this essential part of Askama more easy to
understand, and make future modifications easier.
@Kijewski
Copy link
Collaborator Author

  • I'd like to drop the parse_ prefix on modules within parser, since it seems a bit stuttery to have both.
  • I think we stuff in parse_common could just move into parser/mod.rs directly.
  • I'd prefer to move the contents of parse_target into parse_node.

Done.

  • I'd then also like to move the Expr definition into the expr module and the Node and Target definitions into node, so that we no longer need a separate ast module.

I was thinking that this could be the starting point of a more universal interface if someone wants to implement another syntax, e.g. handlebars (#395). But I guess that's not going to happen anytime soon (not by me, anyway :) ). If someone were to implement another syntax, then they can move the structs into their own file.

@djc
Copy link
Collaborator

djc commented Jan 30, 2023

Nice, thanks!

@djc djc merged commit 7b6f1df into rinja-rs:main Jan 30, 2023
@Kijewski Kijewski deleted the pr-refactor-parser branch January 30, 2023 13:19
@djc djc mentioned this pull request Feb 26, 2023
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