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

fix clippy warnings #581

Merged
merged 4 commits into from
Jun 18, 2021
Merged

fix clippy warnings #581

merged 4 commits into from
Jun 18, 2021

Conversation

jimexist
Copy link
Member

@jimexist jimexist commented Jun 18, 2021

Which issue does this PR close?

Closes #583
Closes #582

Rationale for this change

newest clippy will generate additional warnings on master

What changes are included in this PR?

Are there any user-facing changes?

@@ -248,12 +248,14 @@ where
}

impl ToDFSchema for Schema {
#[allow(clippy::wrong_self_convention)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the suggestion

@jimexist jimexist force-pushed the fix-clippy-warnings branch 3 times, most recently from d1c79cf to 2dfb81e Compare June 18, 2021 09:08
@jimexist jimexist force-pushed the fix-clippy-warnings branch 2 times, most recently from 03b645c to 3bd136c Compare June 18, 2021 10:11
@jimexist
Copy link
Member Author

Maybe for the moment it is wise to temporarily disable coverage task in CI?

@alamb
Copy link
Contributor

alamb commented Jun 18, 2021

Not sure what the coverage failure was: https://github.com/apache/arrow-datafusion/pull/581/checks?check_run_id=2858343109

I restarted the tests

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @jimexist

@@ -248,12 +248,14 @@ where
}

impl ToDFSchema for Schema {
#[allow(clippy::wrong_self_convention)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the suggestion

# 2020-11-15: There is a cargo-tarpaulin regression in 0.17.0
# see https://github.com/xd009642/tarpaulin/issues/618
cargo install --version 0.16.0 cargo-tarpaulin
cargo install --version 0.18.0-alpha3 cargo-tarpaulin
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this change. It seems to cause an error in CI, right?: https://github.com/apache/arrow-datafusion/pull/581/checks?check_run_id=2858343109

Maybe we could put the old version of tarpaulin back?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was failing either way so this line diff was my attempt to fix it in vain

Copy link
Contributor

Choose a reason for hiding this comment

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

I see -- in that case it does seem like disabling code coverage until it is fixed is the right thing to do, and we can file an issue to re-enable it once whatever underlying issue is fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #590 to fix code coverage job. Will disable it via this PR

@jimexist
Copy link
Member Author

Not sure what the coverage failure was: https://github.com/apache/arrow-datafusion/pull/581/checks?check_run_id=2858343109

I restarted the tests

I tried several times and the error persists - not sure why given the fixture is now raw string so escaping should be out of picture

@alamb
Copy link
Contributor

alamb commented Jun 18, 2021

I pushed ac7808b to disable the coverage job. Hopefully the CI checks will then pass and we can get master back to green.

Thank you for the help @jimexist

@alamb
Copy link
Contributor

alamb commented Jun 18, 2021

Thanks again @jimexist

@alamb alamb merged commit 330c809 into apache:master Jun 18, 2021
@houqp houqp added the datafusion Changes in the datafusion crate label Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate
Projects
None yet
3 participants