-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix clippy warnings #581
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,12 +248,14 @@ where | |
} | ||
|
||
impl ToDFSchema for Schema { | ||
#[allow(clippy::wrong_self_convention)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe it's a better idea to remove https://github.com/apache/arrow-datafusion/pull/581/files#diff-54ef5cfee73dd6cdbd9186bbf5931f4e375ccf002f956d1fe8bf35deafaf56a6R244 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand the suggestion |
||
fn to_dfschema(self) -> Result<DFSchema> { | ||
DFSchema::try_from(self) | ||
} | ||
} | ||
|
||
impl ToDFSchema for SchemaRef { | ||
#[allow(clippy::wrong_self_convention)] | ||
fn to_dfschema(self) -> Result<DFSchema> { | ||
// Attempt to use the Schema directly if there are no other | ||
// references, otherwise clone | ||
|
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.
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?
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 was failing either way so this line diff was my attempt to fix it in vain
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.
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
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.
Filed #590 to fix code coverage job. Will disable it via this PR