-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
Conversation
1f4ee68
to
fe5f7ad
Compare
672edfd
to
2822285
Compare
@Kijewski (when) will you have time to review this? Rebasing this will be painful. |
There was a problem hiding this 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> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Nice, thanks! |
Fixes #760, obviates #782.