-
Notifications
You must be signed in to change notification settings - Fork 222
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
Fix Python error name regression #2240
Conversation
visitor.visit_error_name(&mut name); | ||
name | ||
}) | ||
.collect() |
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.
This is one issue, this is a somewhat hacky field that stores error type names when we see them in "[Throws=]" UDL attributes. I think that proc-macros might have handled this correctly, but I'm not sure.
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 like drain
in combination with collect
. I think I could have used something similar here as well!
c682669
to
6cd8464
Compare
@@ -22,7 +22,7 @@ impl CodeType for ErrorCodeType { | |||
} | |||
|
|||
fn canonical_name(&self) -> String { | |||
format!("Type{}", self.id) | |||
format!("Type{}", self.type_label()) |
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.
This is the other issue. There were 2 paths for calculating FFI converters and they didn't use the same logic:
- The ffi_converter_name filter called CodeType::ffi_converter_name, which called
canonical_name
and that usedid
, which is the original name. - But the generated class uses a different ffi_converter_name which had the converted name.
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.
Maybe I only needed to modify ErrorCodeType::canonical_name
, but it seemed safer to change all of the class-like types to use the converted name.
6cd8464
to
811976c
Compare
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 hit this one when trying to upgrade application-services to 0.28.1. It's kind of surprising that this is the first we're hearing of it, but I guess the Python bindings users are a minority.