Skip to content
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: imported optional Map type bindings in Rust #1168

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

krisbitney
Copy link
Contributor

There are two issues affecting imported Map type bindings from compiling in Rust.

First

// expected type option, got map
let mut _headers: Option<Map<String, String>> = Map::<String, String>::new();
let mut _url_params: Option<Map<String, String>> = Map::<String, String>::new();

Second
In the object serialization logic for optional Map type, we are trying to pass a String instead of a Result<String, Error>. Example:

"headers" => {
                reader.context().push(&field, "Option<Map<String, String>>", "type found, reading property");
                _headers = reader.read_optional_ext_generic_map(|reader| {
                    reader.read_string()? //  <---- This `?` should not be here!
                }, |reader| {
                    reader.read_string()
                })?; 
                reader.context().pop();
            }

@cbrzn
Copy link
Contributor

cbrzn commented Aug 21, 2022

nice catch ser 🙏, I previously fixed this but only in object types (#1142) and not for imported object types. I guess you also need to update schema/bind test and we are ready to go

dOrgJelli
dOrgJelli previously approved these changes Aug 21, 2022
Copy link
Contributor

@dOrgJelli dOrgJelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Just need tests to go green

@krisbitney
Copy link
Contributor Author

nice catch ser 🙏, I previously fixed this but only in object types (#1142) and not for imported object types. I guess you also need to update schema/bind test and we are ready to go

Thanks. 😄

@krisbitney
Copy link
Contributor Author

Nice catch! Just need tests to go green

Thanks! 😄

Copy link
Contributor

@Niraj-Kamdar Niraj-Kamdar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krisbitney Is it also fixed for the methodTypes?

@krisbitney
Copy link
Contributor Author

@krisbitney Is it also fixed for the methodTypes?

I fixed both the issues mentioned in the PR description. Are you referring to one of those or something else?

@cbrzn
Copy link
Contributor

cbrzn commented Aug 22, 2022

@krisbitney Is it also fixed for the methodTypes?

I fixed both the issues mentioned in the PR description. Are you referring to one of those or something else?

Just checked it and we are good to go ser

@krisbitney krisbitney merged commit 7d02a54 into origin-dev Aug 22, 2022
@krisbitney krisbitney deleted the fix-imported-optional-map-bindings-rs branch August 22, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants