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

Extract parser #748

Closed
wants to merge 43 commits into from
Closed

Extract parser #748

wants to merge 43 commits into from

Conversation

couchand
Copy link
Contributor

A rough pass at extracting the parser code from the derive generator. A basic AST provides what's needed for the generator, but much more care would need to be given to the public API.

A bare-bones AST printer demonstrates a second client. Slight modifications were made to the parser to ensure all structures could be round-tripped.

Motivations include a personal project to implement an alternate Askama backend, as well as #425 maybe?

@couchand
Copy link
Contributor Author

Questions:

  • threshold question: should the refactor be done?
  • api design question: what should the AST data structures look like?
  • fmt crate question: should this be finished? what should it look like/do, beyond proving the concept?
  • others, i'm sure

@djc
Copy link
Owner

djc commented Nov 23, 2022

What's the goal of the alternate backend?

@couchand
Copy link
Contributor Author

What's the goal of the alternate backend?

More or less #108, and then seeing where things could go from there.

@djc
Copy link
Owner

djc commented Nov 23, 2022

#108 seems a relatively small feature to write an entirely new backend for, though... Why is this so important to you?

So I'm not opposed in principle to exposing the parser API through a separate crate (this actually used to be the case with the askama_shared crate). An AST printer doesn't seem particularly valuable to me; usually the Debug impl of the AST node types we already got is a decent start when pretty-printed.

For a larger change like this, if you want me to review this I'd need a clean commit history with small commits that make one logical change each. I guess my question is, why do you want to do this work upstream rather than just fork it? I have a lot of crates to maintain, and a lot of incoming PRs to review so the question for me is, if you're going out and build your own backend, how much work do you are you effectively asking me to do to support your project, and how is that work valuable to me?

@couchand
Copy link
Contributor Author

This is exactly the kind of discussion I wanted to open this PR to start. My thinking on it is, if it's something you'd generally be doing anyway, I'm happy to help drive it forward. If it's something you see as getting in your way, I won't bother pursuing it.

The fact that it used to be separate might counsel in favor of or against it being a good change to make at this point.


Asides, to answer your various questions:

#108 seems a relatively small feature to write an entirely new backend for, though... Why is this so important to you?

If that's all it were, yes. It's a minor annoyance but not difficult to |safe.

But I think there's opportunity in viewing the model slightly differently, making templates into components... The child template render as not just sticking a string in the right place, really none of it should be. The Askama parser is the first step in a tokenizer for a templated markup parser. (Apologies about the winding and vague explanation, I'm not sure I have a clear idea yet!)

if you want me to review this I'd need a clean commit history with small commits that make one logical change each.

Roger that, sorry to assault you with this terrible history.

why do you want to do this work upstream rather than just fork it?

Short answer, because Askama is awesome! There's a community and tooling and people know it. Better to glom on to all that if possible. Also, it seems like the sort of refactor that's abstractly good to upstream.

how much work do you are you effectively asking me to do to support your project, and how is that work valuable to me?

Totally valid question. First of all, thanks for spearheading this and so many crates (loving quinn btw), and thanks for engaging with my wandering analysis, I'd have written a shorter PR but I didn't have the time :-p

As I mentioned above, if this is going to be a drain on your maintenance then let's close it right away. I'm more than happy to drive the implementation, deal with rebases over changes that happen meanwhile, and of course please tag me if I end up causing you a bug or something. But if you see the refactor being a net-negative on the project long term, close it without prejudice.

As far as the fmt utility is concerned, I really only included that to prove that the implementation was complete. I'm not sure it's worth publishing on its own, but I do want to work toward a more configurable one that I can use to auto-format the legions of templates I've now accumulated. And if one day I could have a tool that auto-applies fixes to all of my templates that would be really cool, too. Anyway, that's the direction I was thinking with this stuff....

@djc
Copy link
Owner

djc commented Dec 12, 2022

I'm probably okay with exposing the parser as a separate crate and I would then be open to reviewing individual changes (one per commit) that attempt to improve the parser API. Past that, not sure.

@couchand
Copy link
Contributor Author

👍 given the immediate future of child care I can confidently say I won't be picking this up again until next year.

In the meantime, if you have any insight or thoughts on the shape of the public API of the parser, please do write them down!

@djc
Copy link
Owner

djc commented Dec 14, 2022

Nope, all those insights and thoughts have been paged out for a while now; I'll await your proposals.

@couchand
Copy link
Contributor Author

Closed in favor of #782

@couchand couchand closed this Feb 28, 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