-
Notifications
You must be signed in to change notification settings - Fork 819
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
run results of cargo --fix
for edition 2021
#714
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.
Looks good to me -- thanks @jimexist
@@ -36,7 +36,7 @@ pub unsafe trait NativeType: | |||
} | |||
|
|||
macro_rules! create_native { | |||
($native_ty:ty,$($impl_pattern:pat)|+) => { | |||
($native_ty:ty,$($impl_pattern:pat_param)|+) => { |
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.
@alamb Either I have something misconfigured locally or this change breaks compilation with the version 1.52.1 configured in rust-toolchain:
error: invalid fragment specifier `pat_param`
--> arrow/src/alloc/types.rs:39:22
|
39 | ($native_ty:ty,$($impl_pattern:pat_param)|+) => {
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= help: valid fragment specifiers are `ident`, `block`, `stmt`, `expr`, `pat`, `ty`, `lifetime`, `literal`, `path`, `meta`, `tt`, `item` and `vis`
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.
@alamb I think we need to somehow pin the rust toolchain
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.
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.
Just noticed rust-toolchain
file is not part of the repo and I only added that locally at some point. That means this was actually a problem on my side, arrow-rs requiring a newer version is totally fine.
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.
@jhorstmann -- I backported this change to the proposed arrow 5.3.0. Given your experience, I think I should probably undo that change and make a new 5.3.0 RC candidate.
@jimexist / @jhorstmann any thoughts?
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.
it's either that or you note in the release notes that the minimal support rust version is 1.53.
rust edition 2021 is likely land in rust 1.56. and given my understanding that arrow does not promise a specific back-compatible yet I feel like that users are supposed to upgrade to latest stable. but my understanding could be wrong.
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.
arrow does not promise a specific back-compatible yet
I now also remember that discussion and agree with it. So from my perspective it is not necessary to revert this change.
We should probably write down this policy in the README if it is not already there yet.
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.
Proposed readme changes: #726
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?