Skip to content
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

ARROW-11603: [Rust] Fix Clippy Lints for Rust 1.50 #9476

Closed
wants to merge 1 commit into from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Feb 11, 2021

Rationale:

CI uses "stable" rust. 1.50 stable was released today: https://blog.rust-lang.org/2021/02/11/Rust-1.50.0.html

The new clippy is pickier resulting in many clippy warnings such as https://github.com/apache/arrow/pull/9469/checks?check_run_id=1881854256

We need to get CI back green

Changes

Based on based on #9475 from @nevi-me, this PR aims to get the CI green as soon as possible.

Ideally, we would fix the actual lint problems, However, when I tried to do so the lints propagated into a significant change -- as clippy says to remove the Result but then there are a bunch of call sites that then also need t be changed

I want to get CI back clean as soon as possible so I just hammered through and cleaned up as best I could as well as sprinkling in various #[allow(clippy::unnecessary_wraps)] as necessary to get a clean run.

My rationale is that the code is no worse than it was before. Though it could be better!

@github-actions
Copy link

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot, @alamb

@alamb
Copy link
Contributor Author

alamb commented Feb 11, 2021

I plan to merge this in as soon as it goes green, unless I hear otherwise. I feel the CI breakage is pretty bad for contributors. I'll try and take a pass through the PRs too to note that there is a CI breakage.

Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Andrew

@alamb
Copy link
Contributor Author

alamb commented Feb 11, 2021

Well, Clippy passed :) but the integration test is still running.

I am about to be away from my keyboard for a few hours -- I'll merge this when I get back if someone doesn't beat me to it,

@codecov-io
Copy link

Codecov Report

Merging #9476 (73befa6) into master (8a968ea) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9476      +/-   ##
==========================================
- Coverage   82.32%   82.31%   -0.02%     
==========================================
  Files         233      233              
  Lines       54446    54414      -32     
==========================================
- Hits        44823    44791      -32     
  Misses       9623     9623              
Impacted Files Coverage Δ
rust/arrow/src/array/builder.rs 84.93% <ø> (ø)
rust/arrow/src/compute/kernels/cast.rs 97.06% <ø> (ø)
rust/arrow/src/compute/kernels/sort.rs 93.56% <ø> (ø)
rust/arrow/src/compute/kernels/substring.rs 98.29% <ø> (ø)
rust/arrow/src/compute/util.rs 98.92% <ø> (ø)
rust/arrow/src/json/reader.rs 82.82% <ø> (ø)
rust/datafusion/src/logical_plan/dfschema.rs 85.52% <ø> (ø)
...atafusion/src/physical_plan/expressions/in_list.rs 75.37% <ø> (ø)
rust/datafusion/tests/sql.rs 99.85% <ø> (ø)
rust/datafusion/tests/user_defined_plan.rs 81.64% <ø> (ø)
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a968ea...fa8dba3. Read the comment docs.

@jorgecarleitao jorgecarleitao changed the title ARROW-11604: [Rust] Fix Clippy Lints for Rust 1.50 ARROW-11603: [Rust] Fix Clippy Lints for Rust 1.50 Feb 11, 2021
@github-actions
Copy link

nevi-me added a commit to nevi-me/arrow that referenced this pull request Feb 13, 2021
# Rationale:

CI uses "stable" rust. 1.50 stable was released today: https://blog.rust-lang.org/2021/02/11/Rust-1.50.0.html

The new clippy is pickier resulting in many clippy warnings such as https://github.com/apache/arrow/pull/9469/checks?check_run_id=1881854256

We need to get CI back green

# Changes
Based on based on apache#9475 from @nevi-me, this PR aims to get the CI green as soon as possible.

Ideally, we would fix the actual lint problems, However, when I tried to do so the lints propagated into a significant change -- as clippy says to remove the `Result` but then there are a bunch of call sites that then also need t be changed

I want to get CI back clean as soon as possible so I just hammered through and cleaned up as best I could as well as sprinkling in various `#[allow(clippy::unnecessary_wraps)]` as necessary to get a clean run.

My rationale is that the code is no worse than it was before. Though it could be better!

Closes apache#9476 from alamb/ARROW-11602-lints

Authored-by: Neville Dipale <nevilledips@gmail.com>
Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
sgnkc pushed a commit to sgnkc/arrow that referenced this pull request Feb 17, 2021
# Rationale:

CI uses "stable" rust. 1.50 stable was released today: https://blog.rust-lang.org/2021/02/11/Rust-1.50.0.html

The new clippy is pickier resulting in many clippy warnings such as https://github.com/apache/arrow/pull/9469/checks?check_run_id=1881854256

We need to get CI back green

# Changes
Based on based on apache#9475 from @nevi-me, this PR aims to get the CI green as soon as possible.

Ideally, we would fix the actual lint problems, However, when I tried to do so the lints propagated into a significant change -- as clippy says to remove the `Result` but then there are a bunch of call sites that then also need t be changed

I want to get CI back clean as soon as possible so I just hammered through and cleaned up as best I could as well as sprinkling in various `#[allow(clippy::unnecessary_wraps)]` as necessary to get a clean run.

My rationale is that the code is no worse than it was before. Though it could be better!

Closes apache#9476 from alamb/ARROW-11602-lints

Authored-by: Neville Dipale <nevilledips@gmail.com>
Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
# Rationale:

CI uses "stable" rust. 1.50 stable was released today: https://blog.rust-lang.org/2021/02/11/Rust-1.50.0.html

The new clippy is pickier resulting in many clippy warnings such as https://github.com/apache/arrow/pull/9469/checks?check_run_id=1881854256

We need to get CI back green

# Changes
Based on based on apache#9475 from @nevi-me, this PR aims to get the CI green as soon as possible.

Ideally, we would fix the actual lint problems, However, when I tried to do so the lints propagated into a significant change -- as clippy says to remove the `Result` but then there are a bunch of call sites that then also need t be changed

I want to get CI back clean as soon as possible so I just hammered through and cleaned up as best I could as well as sprinkling in various `#[allow(clippy::unnecessary_wraps)]` as necessary to get a clean run.

My rationale is that the code is no worse than it was before. Though it could be better!

Closes apache#9476 from alamb/ARROW-11602-lints

Authored-by: Neville Dipale <nevilledips@gmail.com>
Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 2021
# Rationale:

CI uses "stable" rust. 1.50 stable was released today: https://blog.rust-lang.org/2021/02/11/Rust-1.50.0.html

The new clippy is pickier resulting in many clippy warnings such as https://github.com/apache/arrow/pull/9469/checks?check_run_id=1881854256

We need to get CI back green

# Changes
Based on based on apache#9475 from @nevi-me, this PR aims to get the CI green as soon as possible.

Ideally, we would fix the actual lint problems, However, when I tried to do so the lints propagated into a significant change -- as clippy says to remove the `Result` but then there are a bunch of call sites that then also need t be changed

I want to get CI back clean as soon as possible so I just hammered through and cleaned up as best I could as well as sprinkling in various `#[allow(clippy::unnecessary_wraps)]` as necessary to get a clean run.

My rationale is that the code is no worse than it was before. Though it could be better!

Closes apache#9476 from alamb/ARROW-11602-lints

Authored-by: Neville Dipale <nevilledips@gmail.com>
Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants