-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update ion-schema and ion-rs dependencies #151
Conversation
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.
🗺️ Clippy recommended fixes because there was a double reference going on when passing &isl_type_name
.
@@ -436,6 +433,7 @@ mod model_tests { | |||
), | |||
] | |||
.into_iter(), | |||
true, |
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.
🗺️ Adding true
is the only change in this file that's required by the version updates. The others are clippy/fmt fixes.
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 is the true
here? Can you comment it or give it a name? Random booleans in interfaces grow like rust as codebases age, and inhibit readability and maintainability unless you stay on top of it, imo.
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.
ExpandedValueSource::Constructed(annotations, _) => self.inspect_ephemeral_annotations( | ||
depth, | ||
annotations.iter().copied().map(|s| Ok(s.into())), | ||
), | ||
ExpandedValueSource::Constructed(annotations, _) => { | ||
self.inspect_ephemeral_annotations(depth, annotations.iter().copied().map(Ok)) | ||
} |
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 was a Clippy fix.
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.
🗺️ Also a clippy fix.
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.
🗺️ Now that the dependencies are updated, we can refactor the validate command to use the CommandIo
trait like the other commands—but I'm saving that for another day.
@@ -436,6 +433,7 @@ mod model_tests { | |||
), | |||
] | |||
.into_iter(), | |||
true, |
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 is the true
here? Can you comment it or give it a name? Random booleans in interfaces grow like rust as codebases age, and inhibit readability and maintainability unless you stay on top of it, imo.
@@ -496,6 +491,7 @@ mod model_tests { | |||
), | |||
] | |||
.into_iter(), | |||
true, |
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.
Same thing- what's the true
?
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 part of the ion schema model. It is something to do with whether the fields are closed for a given type. I'll create an issue to replace it with an enum.
writer.write_string("Valid")?; | ||
writer.set_field_name("value"); | ||
writer.write_string(element_to_string(&owned_element)?)?; | ||
writer.set_field_name("schema"); | ||
writer.write_string(schema_id)?; | ||
struct_writer.write("result", "Valid")?; | ||
struct_writer.write("value", format!("{}", &owned_element))?; | ||
struct_writer.write("schema", schema_id)?; |
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.
struct_writer
really cleans this up, nice :)
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.
Yes, @zslayton put a lot of effort into trying different approaches in order to find an ergonomic writer API.
Issue #, if available:
None.
Description of changes:
Updates
ion-rs
andion-schema
crates to the latest versions. There's also a bunch of clippy fixes that ended up here too.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.