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 a parser crate #834

Merged
merged 27 commits into from
Jul 31, 2023
Merged

Extract a parser crate #834

merged 27 commits into from
Jul 31, 2023

Conversation

djc
Copy link
Collaborator

@djc djc commented Jul 1, 2023

Fixes #760, obviates #782.

@djc djc requested a review from Kijewski July 1, 2023 14:18
@djc djc force-pushed the parser branch 4 times, most recently from 1f4ee68 to fe5f7ad Compare July 3, 2023 08:19
@djc djc force-pushed the parser branch 5 times, most recently from 672edfd to 2822285 Compare July 3, 2023 08:53
@djc
Copy link
Collaborator Author

djc commented Jul 21, 2023

@Kijewski (when) will you have time to review this? Rebasing this will be painful.

Copy link
Collaborator

@Kijewski Kijewski left a comment

Choose a reason for hiding this comment

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

The changes look great! That's a big improvement in legibility and maintainability.

I went through the commits in chronological order, and looked at the final code. The individual commits look fine, and the result is great.

}

impl<'a> TryFrom<RawSyntax<'a>> for Syntax<'a> {
impl<'a> TryInto<Syntax<'a>> for RawSyntax<'a> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you replace TryFrom with TryInto?

Library authors should usually not directly implement this trait, but should prefer implementing the TryFrom trait, which offers greater flexibility and provides an equivalent TryInto implementation for free, thanks to a blanket implementation in the standard library.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Coherence. At the end of this PR, Syntax no longer lives in askama_derive, so we're not allowed to implement the foreign trait TryFrom for the foreign type Syntax. However, we can implement TryInto for RawSyntax, since that's local. This is explained in the docs for Into, which the TryInto docs link to.

askama_parser/Cargo.toml Show resolved Hide resolved
@Kijewski
Copy link
Collaborator

Maybe the simplest solution would be to overwrite the current head with your branch, and backport #837, #838 and #839 afterwards?

@djc
Copy link
Collaborator Author

djc commented Jul 31, 2023

Maybe the simplest solution would be to overwrite the current head with your branch, and backport #837, #838 and #839 afterwards?

I did it the hard way and properly rebased it instead. :)

@GuillaumeGomez
Copy link
Collaborator

Nice, thanks!

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.

Make askama_shared::parser module public
3 participants