-
Notifications
You must be signed in to change notification settings - Fork 12
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
filters: escape arguments to pluralize
at compile time
#174
filters: escape arguments to pluralize
at compile time
#174
Conversation
d13b5fc
to
5b9a623
Compare
5b9a623
to
d06480f
Compare
d06480f
to
5c34511
Compare
5c34511
to
2744364
Compare
rinja_derive/src/generator.rs
Outdated
buf.write(format_args!("{CRATE}::filters::pluralize(")); | ||
self._visit_arg(ctx, buf, count)?; | ||
for value in [sg, pl] { | ||
buf.write(", "); |
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.
buf.write(", "); | |
buf.write(','); |
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.
👍
rinja_derive/src/generator.rs
Outdated
return None; | ||
}; | ||
match kind { | ||
Some(IntKind::I8) => is_signed_singular(i8::from_str_radix, value, 1, -1), |
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.
I'm trying to think of a way to not have this huge match because it's very error prone to have to ensure that everything is working out correctly... Isn't it be possible somehow to have an impl with an associated type? Another (easier) way is to write a macro where we list types and then everything is generated correctly.
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.
I added a macro, but it's not much less ugly.
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.
Indeed but at least less error-prone, well done!
rinja_derive/src/generator.rs
Outdated
Some(IntKind::I64) => is_signed_singular(i64::from_str_radix, value, 1, -1), | ||
Some(IntKind::I128) => is_signed_singular(i128::from_str_radix, value, 1, -1), | ||
Some(IntKind::Isize) => { | ||
if cfg!(target_pointer_width = "16") { |
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 not doing isize::from_str_radix
?
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 target and host pointer widths may be different.
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.
Ah ok, cross-compilation was the issue.
Thanks for the improvement! |
Thanks! :) Should have thought about that already in the first PR. |
Well I definitely didn't think about that so that makes two of us. |
We can tell at compile time how to escape
""
or"s"
. The former does not need to write anything, the latter does not need to be escaped.