-
Notifications
You must be signed in to change notification settings - Fork 26
Fix build (and upgrade arrow) #139
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
base: main
Are you sure you want to change the base?
Conversation
@thief-sty Thanks for the PR, I will have a look at it towards the end of the week. |
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.
Thanks for updating the dependencies. These major version bumps in arrow are quite an annoyance.
Would you mind addressing the linting issue so that we get a green build? Thanks
&self, | ||
_request: Request<FlightDescriptor>, | ||
) -> FlightResult<Response<PollInfo>> { | ||
Err(Status::unimplemented("Not yet implemented")) |
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.
👍
@thief-sty I did continue a bit on your changes. Fixing the current Unless you have a better proposal, I am fine with allowing the lint for the time being in #![allow(result_large_err)] and removing that line once arrow upgraded to tonic 0.14. The remaining error is a deprecation warning, which you can fix by: fn try_from(descriptor: FlightDescriptor) -> Result<Self, Self::Error> {
match DescriptorType::try_from(descriptor.r#type) {
Err => Err(Status::invalid_argument(format!(
"unsupported descriptor type: {}",
descriptor.r#type
))),
Ok(DescriptorType::Cmd) => {
serde_json::from_slice::<Self>(&descriptor.cmd).map_err(from_json_error)
}
Ok(descriptor_type) => Err(Status::invalid_argument(format!(
"Expected command, got {descriptor_type:?}"
))),
}
} After that, |
Bumping this just in case you didn't see my commits from last week |
This closes #138 by upgrading arrow to 56.0.0 (latest).