-
Notifications
You must be signed in to change notification settings - Fork 11
set msrv via Cargo.toml, use 2024 edition #152
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
Conversation
Cargo.toml
Outdated
edition = "2021" | ||
edition = "2024" | ||
rust-version = "1.86.0" |
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.
Our intention here is rather than enforcing ourselves a rust-version
in Cargo.toml, let the datafusion
crate enforce it instead. Automatically, because we are depending on datafusion
, it's not necessary for us to apply further restrictions on the MSRV.
What we need to do though is to make sure that developers contributing to this project use exactly DataFusion's MSRV, therefore it's important to keep the rust-toolchain.toml
file that was introduced in #72.
Any chance to keep the rust-toolchain.toml
while changing it to the new 1.86.0
channel?
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.
Done!
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.
Nice! just some minor comments, but otherwise very grateful for this 😄
|
||
impl DistributedCodec { | ||
pub fn new_combined_with_user(cfg: &SessionConfig) -> impl PhysicalExtensionCodec { | ||
pub fn new_combined_with_user(cfg: &SessionConfig) -> impl PhysicalExtensionCodec + use<> { |
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.
👀 use<>
. Is this a thing in Rust? no idea what this means
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, it means that the lifetime of the returned impl doesn't depend on the &SessionConfig
rust-toolchain.toml
Outdated
@@ -1,3 +1,3 @@ | |||
[toolchain] | |||
channel = "1.85.1" | |||
channel = "1.86.0" |
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.
🤔 Now that I think about it, probably it makes sense to leave this as 1.85.1 in this PR, as this is still running on DataFusion 49, and we need to make sure that both pipelines and developers adhere to support at least 1.85.1.
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.
okay done
@gabotechs can you trigger CI so we can see if everything passes? |
Nice, thanks @adriangb! |
The diff is mostly running
cargo fmt
on 2024 edition.