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

Update to [email protected] and remove re-export of ion-rs types #220

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

popematt
Copy link
Contributor

@popematt popematt commented Oct 7, 2024

Issue #, if available:

#137

Description of changes:

  • Updates to [email protected]
  • This update broke the implementations of WriteToIsl, so I removed the WriteToIsl trait in favor of implementing WriteAsIon
  • Updates ion-schema-tests to the latest commit. (Required because ion-rs supports only up to u128 for integers.)
  • Minor refactoring in ranges.rs to use Min and Max instead of Unbounded. That made it cleaner to implement WriteAsIon.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@zslayton zslayton left a comment

Choose a reason for hiding this comment

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

🚀

ion-schema/src/ion_extension.rs Outdated Show resolved Hide resolved
_ => None,
}
}
fn as_u64(&self) -> Option<u64> {
match self.value() {
Value::Int(Int::I64(i)) => i.to_u64(),
Value::Int(Int::BigInt(i)) => i.to_u64(),
Value::Int(i) => i.as_i128()?.to_u64(),
Copy link
Contributor

Choose a reason for hiding this comment

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

...but weirdly, there's not an Int::as_u64() method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tracking this as amazon-ion/ion-rust#840.

ion-schema/src/isl/mod.rs Outdated Show resolved Hide resolved
ion-schema/src/isl/isl_type.rs Outdated Show resolved Hide resolved

// write the previously constructed ISL model into a schema file using `write_to`
let write_schema_result = expected_schema.write_to(&mut writer);
let write_schema_result = expected_schema.write_as_ion(&mut writer);
Copy link
Contributor

Choose a reason for hiding this comment

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

This works, though it can also be written writer.write(expected_schema).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't be written that way because IslSchema does not implement WriteAsIon. This is because WriteAsIon implementations are allowed to write a single value, but an IslSchema is a sequence of top-level values.

ion-schema/src/isl/ranges.rs Outdated Show resolved Hide resolved
Ok(())
}
Limit::Inclusive(value) if self.lower == self.upper => value.fmt(f),
_ => f.write_fmt(format_args!("range::[{},{}]", self.lower, self.upper)),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is my first time seeing a manual invocation of format_args! in the wild. 🤔

@@ -131,7 +131,7 @@ impl BuiltInTypeDefinition {
let mut constraints = vec![];

// parses an isl_type to a TypeDefinition
let type_name = isl_type.name();
let type_name = isl_type.name().map(|x| x.to_owned());
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL that Option<&str> doesn't implement ToOwned.

@popematt popematt merged commit 8fe6994 into amazon-ion:main Oct 7, 2024
3 checks passed
@popematt popematt deleted the update-ion-rs-b branch October 7, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants