You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/02/16 20:03:49 UTC
[GitHub] [arrow-rs] gsserge opened a new pull request #1324: Enable dead_code lint
gsserge opened a new pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324
# Which issue does this PR close?
Closes https://github.com/apache/arrow-rs/issues/1255
# Rationale for this change
It's beneficial to run clippy and compiler lints as strict as possible.
# What changes are included in this PR?
This PR drops `#![allow(dead_code)]`, which is the last top level `allow` attribute in the `arrow` crate. Unfortunately, it's the most difficult one to deal with, because there's a lot of code that exists and sometimes even has tests, but it's neither pub exported nor used anywhere. I don't feel comfortable removing it (except for the obvious cases like the unused time consts), that's why I've decided to use local `#[allow(dead_code)]` instead. The existing code is left intact, because it might be needed in the future, while overall the linting process gets more strict.
# Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] gsserge commented on pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
gsserge commented on pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#issuecomment-1042359938
Cool that enabling this lint reveals real issues, so it is actually very beneficial for everyone to be careful with `allow`'s and use them sparingly. However, I'd prefer not to expand the scope of this PR, and fix the issues in separate PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#issuecomment-1042643832
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1324?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1324](https://codecov.io/gh/apache/arrow-rs/pull/1324?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3b07835) into [master](https://codecov.io/gh/apache/arrow-rs/commit/827cc3e9e153d6cf4e6a4f8a71c760043a9f25a8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (827cc3e) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 3b07835 differs from pull request most recent head f8d90fd. Consider uploading reports for the commit f8d90fd to get more accurate results
[](https://codecov.io/gh/apache/arrow-rs/pull/1324?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1324 +/- ##
==========================================
+ Coverage 83.00% 83.03% +0.02%
==========================================
Files 180 180
Lines 52919 52919
==========================================
+ Hits 43924 43939 +15
+ Misses 8995 8980 -15
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1324?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/array/array\_dictionary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X2RpY3Rpb25hcnkucnM=) | `91.89% <ø> (+3.60%)` | :arrow_up: |
| [arrow/src/array/array\_primitive.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3ByaW1pdGl2ZS5ycw==) | `94.69% <ø> (ø)` | |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `86.73% <ø> (ø)` | |
| [arrow/src/array/data.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2RhdGEucnM=) | `83.30% <ø> (ø)` | |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `84.65% <ø> (+0.26%)` | :arrow_up: |
| [arrow/src/compute/util.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUvdXRpbC5ycw==) | `98.90% <ø> (ø)` | |
| [arrow/src/csv/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2Nzdi9yZWFkZXIucnM=) | `88.12% <ø> (ø)` | |
| [arrow/src/csv/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2Nzdi93cml0ZXIucnM=) | `72.13% <ø> (ø)` | |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `84.53% <ø> (ø)` | |
| [arrow/src/ipc/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2lwYy93cml0ZXIucnM=) | `83.45% <ø> (-0.04%)` | :arrow_down: |
| ... and [5 more](https://codecov.io/gh/apache/arrow-rs/pull/1324/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1324?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1324?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [827cc3e...f8d90fd](https://codecov.io/gh/apache/arrow-rs/pull/1324?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] gsserge commented on a change in pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
gsserge commented on a change in pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#discussion_r808744019
##########
File path: arrow/src/csv/writer.rs
##########
@@ -97,6 +97,7 @@ pub struct Writer<W: Write> {
/// The object to write to
writer: csv_crate::Writer<W>,
/// Column delimiter. Defaults to `b','`
+ #[allow(dead_code)]
Review comment:
https://github.com/apache/arrow-rs/issues/1328
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Dandandan commented on a change in pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#discussion_r808432653
##########
File path: arrow/src/csv/reader.rs
##########
@@ -1055,6 +1056,7 @@ pub struct ReaderBuilder {
/// The default batch size when using the `ReaderBuilder` is 1024 records
batch_size: usize,
/// The bounds over which to scan the reader. `None` starts from 0 and runs until EOF.
+ #[allow(dead_code)]
Review comment:
Is this a bug?
##########
File path: arrow/src/csv/writer.rs
##########
@@ -97,6 +97,7 @@ pub struct Writer<W: Write> {
/// The object to write to
writer: csv_crate::Writer<W>,
/// Column delimiter. Defaults to `b','`
+ #[allow(dead_code)]
Review comment:
This seems a bug (not to use the delimiter option) . I think we should add a note or fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb merged pull request #1324: Fix clippy lint `dead_code`
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#discussion_r808461410
##########
File path: arrow/src/ipc/writer.rs
##########
@@ -535,6 +535,7 @@ pub struct StreamWriter<W: Write> {
/// IPC write options
write_options: IpcWriteOptions,
/// A reference to the schema, used in validating record batches
+ #[allow(dead_code)]
Review comment:
I wonder if we can simply delete this field? I can take a stab at it in a follow on PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#discussion_r808993500
##########
File path: arrow/src/array/array_dictionary.rs
##########
@@ -536,6 +536,7 @@ mod tests {
assert!(iter.next().is_none());
}
+ #[test]
Review comment:
👍 nice fix
##########
File path: arrow/src/csv/writer.rs
##########
@@ -107,6 +108,7 @@ pub struct Writer<W: Write> {
/// The timestamp format for timestamp arrays
timestamp_format: String,
/// The timestamp format for timestamp (with timezone) arrays
Review comment:
fyi @sum12
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#issuecomment-1042896353
> Cool that enabling this lint reveals real issues, so it is actually very beneficial for everyone to be careful with allow's and use them sparingly. However, I'd prefer not to expand the scope of this PR, and fix the issues in separate PRs.
I think this is a great strategy. Thank you @gsserge for filing the follow on issues
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] gsserge commented on a change in pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
gsserge commented on a change in pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#discussion_r808483707
##########
File path: arrow/src/csv/reader.rs
##########
@@ -1055,6 +1056,7 @@ pub struct ReaderBuilder {
/// The default batch size when using the `ReaderBuilder` is 1024 records
batch_size: usize,
/// The bounds over which to scan the reader. `None` starts from 0 and runs until EOF.
+ #[allow(dead_code)]
Review comment:
To me it looks more like an omission in the builder. `Reader::from_csv_reader()` correctly processes the `bounds` parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] gsserge commented on a change in pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
gsserge commented on a change in pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#discussion_r808734662
##########
File path: arrow/src/csv/reader.rs
##########
@@ -1055,6 +1056,7 @@ pub struct ReaderBuilder {
/// The default batch size when using the `ReaderBuilder` is 1024 records
batch_size: usize,
/// The bounds over which to scan the reader. `None` starts from 0 and runs until EOF.
+ #[allow(dead_code)]
Review comment:
https://github.com/apache/arrow-rs/issues/1327
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #1324: Fix clippy lint `dead_code`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#issuecomment-1042908422
Here is a PR that removes some more of this discovered dead code: https://github.com/apache/arrow-rs/pull/1331
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] gsserge commented on a change in pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
gsserge commented on a change in pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#discussion_r808462958
##########
File path: arrow/src/ipc/writer.rs
##########
@@ -535,6 +535,7 @@ pub struct StreamWriter<W: Write> {
/// IPC write options
write_options: IpcWriteOptions,
/// A reference to the schema, used in validating record batches
+ #[allow(dead_code)]
Review comment:
That would be awesome :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] gsserge commented on a change in pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
gsserge commented on a change in pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#discussion_r808726012
##########
File path: arrow/src/ipc/writer.rs
##########
@@ -535,6 +535,7 @@ pub struct StreamWriter<W: Write> {
/// IPC write options
write_options: IpcWriteOptions,
/// A reference to the schema, used in validating record batches
+ #[allow(dead_code)]
Review comment:
The field is removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] gsserge commented on a change in pull request #1324: Enable dead_code lint
Posted by GitBox <gi...@apache.org>.
gsserge commented on a change in pull request #1324:
URL: https://github.com/apache/arrow-rs/pull/1324#discussion_r808491994
##########
File path: arrow/src/csv/writer.rs
##########
@@ -97,6 +97,7 @@ pub struct Writer<W: Write> {
/// The object to write to
writer: csv_crate::Writer<W>,
/// Column delimiter. Defaults to `b','`
+ #[allow(dead_code)]
Review comment:
Yep, this time the delimiter is configured via the builder, but gets completely ignored
```rust
impl<W: Write> Writer<W> {
/// Create a new CsvWriter from a writable object, with default options
pub fn new(writer: W) -> Self {
let delimiter = b',';
let mut builder = csv_crate::WriterBuilder::new();
let writer = builder.delimiter(delimiter).from_writer(writer);
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org