-
Notifications
You must be signed in to change notification settings - Fork 33
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
Mass renaming, container Display impls #486
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.
To be completely honest, I did not read all of the changes. I agree with the overview of the renaming changes, but I did not read through those commits because of their sheer size. I did, however, look at the Display
-related commits and the clippy commits. 👍
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #486 +/- ##
==========================================
+ Coverage 89.45% 89.48% +0.02%
==========================================
Files 77 77
Lines 13683 13711 +28
==========================================
+ Hits 12240 12269 +29
+ Misses 1443 1442 -1
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
A few lost patch fragments in the wrong commits, but I stopped noting them after this shipped :P
Null(IonType::SExpression), | ||
Null(IonType::SExp), |
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 are you doing here? You belong in the previous commit!
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.
😅 Yeah, there were a few of these. I really need to set up a pre-commit git hook for cargo test --all-features
to prevent this sort of thing.
|
||
#[test] | ||
fn list_display_roundtrip() { | ||
let list = ion_list![1, 2, 3, true, false]; | ||
|
||
// Use the Display impl to serialize the list to text | ||
let text_list = format!("{list}"); | ||
// Parse the result and make sure it represents the same data | ||
let expected_element: Element = list.into(); | ||
let actual_element = Element::read_one(text_list).unwrap(); | ||
assert!(expected_element.ion_eq(&actual_element)); | ||
} | ||
|
||
#[test] | ||
fn sexp_display_roundtrip() { | ||
let sexp = ion_sexp! (1 2 3 true false); | ||
|
||
// Use the Display impl to serialize the sexp to text | ||
let text_sexp = format!("{sexp}"); | ||
// Parse the result and make sure it represents the same data | ||
let expected_element: Element = sexp.into(); | ||
let actual_element = Element::read_one(text_sexp).unwrap(); | ||
assert!(expected_element.ion_eq(&actual_element)); | ||
} |
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 just can't get over how neat these macros are. Great work, Zack and Matt!
@@ -309,7 +309,7 @@ mod tests { | |||
ops: vec![AsBool], | |||
op_assert: Box::new(|e: &Element| { | |||
let expected = Element::from(true); | |||
assert_eq!(Some(true), e.as_boolean()); |
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 change is lost too, you belong in the bool
rename commit.
(AsBool, Box::new(|e| assert_eq!(None, e.as_boolean()))), | ||
(AsBool, Box::new(|e| assert_eq!(None, e.as_bool()))), |
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.
Lost
pub fn as_boolean(&self) -> Option<bool> { | ||
pub fn as_bool(&self) -> Option<bool> { |
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.
Ah, looks like we picked up an additional rename here.
Builds on the changes in PR #485. After that's merged, I'll update this to target
main
.The diff contains no logic changes.
This PR performs several superficial cleanup tasks, including:
crate::value
module tocrate::element
Boolean
->Bool
Integer
->Int
UInteger
->UInt
SExpression
->SExp
(this rename was started in an earlier PR, but additional references to the type name were found)Display
impls forList
,SExp
, andStruct
(addressing ImplementDisplay
forvalue::owned::Struct
,value::owned::Sequence
, etc. #447)Display
impl forValue
so it can eventually be used independent ofElement
.Display
forIonType
to use official type namesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.