-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Adds better exception macro #2897
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
Conversation
} | ||
|
||
extends_exception! { | ||
PyKeyboardInterrupt, |
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.
What I don't like here is that I have to repeat ident
and literal
names. Why?
- I cannot use just
ident
names with#[pyclass(name = ..., base = ...)]
, because they explicitly needliteral
strings. I even cannot usestringify!
, because it is not expanded in#[pyclass]
attributes - I cannot use just
literal
, because Rust cannotunstrigify!
things, unless https://docs.rs/unstringify/0.1.1/unstringify/macro.unstringify.html is installed
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.
Friendly ping @youknowone @DimitrisJim
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 don't any a nice idea for this. This problem is known to be a tranditional rust macro problem. So it looks just ok to me for now. Will it be better if pyclass takes both ident and str for its macro arguments? @coolreader18
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 think so, #[pyclass(name = :ident | :literal, base = :ident | :literal)]
seems like a good solution 👍
I will implement it in a separate PR.
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.
Looks like that there's nothing I can do. It is a limitation of pub type AttributeArgs = Vec<NestedMeta>;
, where NestedMeta
can be:
/// ## Path
///
/// A meta path is like the `test` in `#[test]`.
///
/// ## List
///
/// A meta list is like the `derive(Copy)` in `#[derive(Copy)]`.
///
/// ## NameValue
///
/// A name-value meta is like the `path = "..."` in `#[path =
/// "sys/windows.rs"]`.
And NameValue
only allows Lit
values 😞
So, I guess that the only way is to pass ident
and literal
as two separate params.
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.
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.
Hm #[pyexception($class_name, $base_class)]
seems to work, I will continue my experiments 👍
vm/src/exceptions.rs
Outdated
} | ||
|
||
extends_exception! { | ||
PyKeyboardInterrupt, |
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.
Btw, here's another example why I need #2876
As you can see it uses 2 spaces and cargo fmt
ignores it.
f261d1d
to
b5c03c6
Compare
@@ -109,6 +109,7 @@ fn new_module_item( | |||
inner: ContentItemInner { index, attr_name }, | |||
pyattrs: pyattrs.unwrap_or_else(Vec::new), | |||
}), | |||
"pyexception" => unreachable!("#[pyexception] {:?}", pyattrs.unwrap_or_else(Vec::new)), |
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.
Test that it is not counted in module
, I can leave it or remove it.
@youknowone looks like a nice solution to me, what do you think? |
Am I moving in the right direction? 🙂 |
I am asking because I am also thinking about this alternative and easier solution from a725e2b It just enhances |
@coolreader18 could you review this PR? |
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.
LGTM. I've been thinking for a while about what to do with our inheritance situation, and though we're not there yet with e.g. OSError having fields of its own as well as Exception's, this will make it easy to add once we get there.
Oh no! My bad, I should've marked it as "WIP".
Considering |
Closes #2771
I've changed a single exception to show my progress. If this is ok, then I can change all of them.