Add quote integration as optional printing feature #13
Conversation
|
@LukasKalbertodt Do you have the option to look at this ? |
|
I will, but I'm currently short on time. |
|
All good, take the time you need |
|
Hi you got the time to merge it? |
| version = "0.4.0" | ||
| version = "0.4.1" |
There was a problem hiding this comment.
I will bump the version as part of my release process. At least in my workflow, this doesn't belong into feature commits.
| default = ["proc-macro2"] | ||
| default = ["proc-macro2", "printing"] |
There was a problem hiding this comment.
Don't make it a default feature. In fact, I consider removing proc-macro2 from the default features as well.
| proc-macro2 = { version = "1", optional = true } | ||
| proc-macro2 = { version = "1", optional = true } | ||
| unicode-xid = { version = "0.2.4", optional = true } | ||
| quote = { version = "1", optional = true, no-default-features = true } |
There was a problem hiding this comment.
Please don't align it like that. Just one space after the comma. and before the =.
| check_suffix = ["unicode-xid"] | ||
| printing = ["dep:quote"] |
There was a problem hiding this comment.
Mh this feature requires Rust 1.60, but currently this is targetting 1.54. So if we want this, I would only release that with a major version bump. While I dislike implicit features and do think dep: is a step in the right direction, I think this isn't worth it. So please change this to just ["quote"].
Also, independently: I guess this should also mention "proc-macro2" as that's required to implement the printing feature.
| ($id:ident) => { | ||
| impl<B: Buffer + Clone> ToTokens for crate::$id<B> { | ||
| fn to_tokens(&self, tokens: &mut proc_macro2::TokenStream) { | ||
| tokens.append(<Self as Into<proc_macro2::Literal>>::into(self.clone())) |
There was a problem hiding this comment.
That shouldn't require a clone. I suppose there should be From<&Literal> for proc_macro2::Literal impls hu? The only thing the current impl (for non references) does is call parse on the raw_input(). That works with a reference as well. Can you add these impls in a separate commit? And then you can use them here and remove the Clone bound. If you prefer I can also add those impls.
Also, tokens.append(proc_macro2::Literal::from(self)) should work as well?
| fn into_token_stream(self) -> proc_macro2::TokenStream | ||
| where | ||
| Self: Sized, | ||
| { | ||
| proc_macro2::TokenStream::from(proc_macro2::TokenTree::from(<Self as Into< | ||
| proc_macro2::Literal, | ||
| >>::into(self))) | ||
| } |
There was a problem hiding this comment.
And I suppose if we remove the Clone bound, this also can be removed.
| use crate::BoolLit::*; | ||
| tokens.append(proc_macro2::Ident::new( | ||
| match self { | ||
| True => "true", | ||
| False => "false", | ||
| }, | ||
| proc_macro2::Span::call_site(), | ||
| )) |
There was a problem hiding this comment.
| use crate::BoolLit::*; | |
| tokens.append(proc_macro2::Ident::new( | |
| match self { | |
| True => "true", | |
| False => "false", | |
| }, | |
| proc_macro2::Span::call_site(), | |
| )) | |
| tokens.append(proc_macro2::Ident::from(self)); |
Should work?
| let other = crate::BoolLit::True | ||
| .to_token_stream() | ||
| .into_iter() | ||
| .next() | ||
| .and_then(|v| { | ||
| if let TokenTree::Ident(v) = v { | ||
| Some(v) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap(); | ||
| assert_eq!(other, "true") |
There was a problem hiding this comment.
| let other = crate::BoolLit::True | |
| .to_token_stream() | |
| .into_iter() | |
| .next() | |
| .and_then(|v| { | |
| if let TokenTree::Ident(v) = v { | |
| Some(v) | |
| } else { | |
| None | |
| } | |
| }) | |
| .unwrap(); | |
| assert_eq!(other, "true") | |
| let v = crate::BoolLit::True.to_token_stream().into_iter().collect::<Vec<_>>(); | |
| assert_eq!(v.len(), 1); | |
| let TokenTree::Ident(ident) = v[0] else { panic!("not an ident") }; | |
| assert_eq!(ident, "true"); |
How about this? This also checks that there are no extra token trees in the stream.
| .and_then(|v| { | ||
| if let TokenTree::Literal(v) = v { | ||
| Some(v) | ||
| } else { | ||
| None | ||
| } | ||
| }) |
There was a problem hiding this comment.
That shouldn't be necessary as a TryFrom impl also exists for TokenTree, not only Literal, right?
I have been working with
litrsin one of my projects for quite some time and started to get annoyed that there was noquoteintegration. To fix this, I have now added a very simplequoteintegration as an optional feature. Does this align with the project's views as light weight? I took this into consideration when implementing it by adding it as its own feature flag.NOTE: the only contentful commit in this case is Add quote printing as an optional feature. The commit Run cargo fmt was litterally just running rust fmt and commiting it.
Is there anything im missing for this to be merged into master?