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

feature gate csv functionality #312

Merged
merged 5 commits into from
May 21, 2021
Merged

Conversation

ritchie46
Copy link
Contributor

@ritchie46 ritchie46 commented May 17, 2021

Which issue does this PR close?

This PR puts the csv parsing logic behind a feature gate. Thereby removing csv crate as a dependency. This closes #309.

I also added the csv feature to the default features to reduce the probability of unexpected changes for users that relied on the default features.

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 @ritchie46 !

I wonder if we should test this in CI so that it doesn't break? When I ran the tests it seems as if one of the doc tests fails without this feature

cargo test -p arrow --no-default-features
---- src/util/string_writer.rs - util::string_writer (line 25) stdout ----
error[E0432]: unresolved import `arrow::csv`
 --> src/util/string_writer.rs:27:5
  |
5 | use arrow::csv;
  |     ^^^^^^^^^^ no `csv` in the root

error: aborting due to previous error

For more information about this error, try `rustc --explain E0432`.
Couldn't compile the test.

failures:
    src/util/string_writer.rs - util::string_writer (line 25)

test result: FAILED. 87 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 6.20s

error: test failed, to rerun pass '--doc'
(arrow_dev) alamb@ip-10-0-0-124:~/Software/arrow-rs$ 

@alamb alamb added the enhancement Any new improvement worthy of a entry in the changelog label May 17, 2021
@ritchie46
Copy link
Contributor Author

I wonder if we should test this in CI so that it doesn't break? When I ran the tests it seems as if one of the doc tests fails without this feature

Oh, I missed that one. Will fix that and I will see if I can add a CI change.

@codecov-commenter
Copy link

Codecov Report

Merging #312 (a741a99) into master (c863a2c) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
- Coverage   82.52%   82.52%   -0.01%     
==========================================
  Files         162      162              
  Lines       44007    44007              
==========================================
- Hits        36316    36315       -1     
- Misses       7691     7692       +1     
Impacted Files Coverage Δ
arrow/src/csv/reader.rs 88.36% <ø> (ø)
arrow/src/csv/writer.rs 82.64% <ø> (ø)
arrow/src/error.rs 8.88% <ø> (ø)
arrow/src/util/string_writer.rs 73.33% <ø> (ø)
parquet/src/encodings/encoding.rs 94.85% <0.00%> (-0.20%) ⬇️

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 c863a2c...a741a99. Read the comment docs.

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.

I love it -- this looks great. Thanks @ritchie46

Thoughts @andygrove / @jorgecarleitao / @nevi-me ?

@alamb alamb merged commit dde86b9 into apache:master May 21, 2021
@alamb alamb added arrow Changes to the arrow crate and removed enhancement Any new improvement worthy of a entry in the changelog labels Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

csv feature gate
3 participants