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

Add csv driver option to specify csv read args #93

Merged
merged 5 commits into from
Oct 7, 2022

Conversation

MaxSchambach
Copy link
Contributor

Description

Adds a read_csv_kwargs argument to CsvDriver initialization which is used in all read_csv calls in the class.
Does not break backward compatibility, as get_df and get_iter still allow to specify kwargs for read_csv which will take precedence over the ones given at initialization.

This makes the creation of new catalog entries based on the CsvDriver much easier as dataset-specific read options (such as seperator, dtypes, etc.) can be specified in the driver_kwargs.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring including code style reformatting
  • Other (please describe):

Checklist:

  • I have read the contributing guideline doc (external contributors only)
  • Lint and unit tests pass locally with my changes
  • I have kept the PR small so that it can be easily reviewed
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • All dependency changes have been reflected in the pip requirement files.

- Allows for more flexible usage of CsvDriver in catalog
@github-actions
Copy link

github-actions bot commented Oct 6, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@MaxSchambach
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

requirements.txt Outdated
@@ -462,62 +462,6 @@ google-resumable-media==2.3.3 \
googleapis-common-protos==1.56.4 \
--hash=sha256:8eb2cbc91b69feaf23e32452a7ae60e791e09967d81d4fcc7fc388182d1bd394 \
--hash=sha256:c25873c47279387cfdcbdafa36149887901d36202cb645a0e4f29686bf6e4417
greenlet==1.1.2 \
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't a change here mean that requirements.in also got changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dang, I didn't do any requirements changes and did not commit that on purpose. Will revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for the catch!

@MaxSchambach
Copy link
Contributor Author

I don't understand, why the test fails due to changes introduced by black. I did run black locally before submitting and don't see why it fails in the cloud test. Any ideas @pzdkn ?

@MaxSchambach
Copy link
Contributor Author

I don't understand, why the test fails due to changes introduced by black. I did run black locally before submitting and don't see why it fails in the cloud test. Any ideas @pzdkn ?

should be solved now. weirdly, pre-commit performed a different black call than calling black directly.

Copy link
Contributor

@pzdkn pzdkn left a comment

Choose a reason for hiding this comment

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

Thanks Max, LGTM!

@MaxSchambach MaxSchambach merged commit b2f3958 into main Oct 7, 2022
@MaxSchambach MaxSchambach deleted the max-csv-driver-update branch October 7, 2022 14:21
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants