Do not include the double quotes in #[pyclass(name = "literal")]
#1303
Conversation
pyo3-derive-backend/src/pyclass.rs
Outdated
| @@ -15,7 +15,7 @@ use syn::{parse_quote, Expr, Token}; | |||
| /// The parsed arguments of the pyclass macro | |||
| pub struct PyClassArgs { | |||
| pub freelist: Option<syn::Expr>, | |||
| pub name: Option<syn::Expr>, | |||
| pub name: Option<syn::ExprPath>, | |||
There was a problem hiding this comment.
I actually wonder if this should be syn::LitStr?
There was a problem hiding this comment.
Could easily be LitStr if we deprecate the other branch as you said, although it may be better not to allow arbitrary strings, so maybe Ident is even better.
pyo3-derive-backend/src/pyclass.rs
Outdated
| syn::Expr::Path(exp) if exp.path.segments.len() == 1 => { | ||
| self.name = Some(exp.clone().into()); | ||
| self.name = Some(exp.clone()); | ||
| } |
There was a problem hiding this comment.
I think this is the branch for name without quotes? I would actually be quite happy to deprecate this as I think name="CustomName" is clearer than name=CustomName.
If you agree, we could make this branch return an error message suggesting the user wrap the name in quotes.
cc @kngwyu - do you agree with a breaking change here to tidy this up?
There was a problem hiding this comment.
I’d support that as well, also ExprPath is actually too restrictive as it doesn’t play well with substituted macro variables which are basically None delimited ExprGroup. That was the very reason for my initial PR.
There was a problem hiding this comment.
Yeah, I agree to remove the branch.
pyo3-derive-backend/src/pyclass.rs
Outdated
| syn::Expr::Path(exp) if exp.path.segments.len() == 1 => { | ||
| self.name = Some(exp.clone().into()); | ||
| self.name = Some(exp.clone()); | ||
| } |
There was a problem hiding this comment.
Yeah, I agree to remove the branch.
|
@scalexm I'm going to finish and force push this branch this morning, so we can include it in the release. Hope that's okay! |
|
@kngwyu this branch should now be ready to merge for the release! |
|
Thanks! |
Embarrassing... My previous PR was quite wrong actually, as it would set the class name to
'"literal"', double quotes included :)