-
Notifications
You must be signed in to change notification settings - Fork 791
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
Include field name in merge error message #3113
Conversation
arrow-schema/src/field.rs
Outdated
let result = field | ||
.try_merge(&Field::new("c1", DataType::Float32, true)) | ||
.expect_err("should fail"); | ||
assert_eq!("Schema error: Fail to merge schema field 'c1' because the from data_type = Float32 does not equal Int64", format!("{}", result)); |
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.
assert_eq!("Schema error: Fail to merge schema field 'c1' because the from data_type = Float32 does not equal Int64", format!("{}", result)); | |
assert_eq!("Schema error: Fail to merge schema field 'c1' because the from data_type = Float32 does not equal Int64", "{}", result); |
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.
That doesn't seem to be valid?
error: format argument must be a string literal
--> arrow-schema/src/field.rs:460:133
|
460 | assert_eq!("Schema error: Fail to merge schema field 'c1' because the from data_type = Float32 does not equal Int64", "{}", result);
| ^^^^^^
```
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.
Oh sorry, brain fart result.to_string()
perhaps?
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 took the liberty of applying this, so that we can get this in. Tbh I'm suprised clippy isn't complaining about this - https://rust-lang.github.io/rust-clippy/master/#useless_format
Benchmark runs are scheduled for baseline = 371ec57 and contender = c99d2f3. c99d2f3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* Include field name in merge error message * Lint Co-authored-by: Raphael Taylor-Davies <r.taylordavies@googlemail.com>
* Include field name in merge error message * Lint Co-authored-by: Raphael Taylor-Davies <r.taylordavies@googlemail.com>
Which issue does this PR close?
Closes #3095
Rationale for this change
Follow on from #3098 to include field name in error messages
What changes are included in this PR?
Are there any user-facing changes?