Skip to content

In Which We Flush Buffers After Finishing Writing to Them #246

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 20, 2018
Merged

In Which We Flush Buffers After Finishing Writing to Them #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
pass in tables like schema."tablename.with.periods".
- Adding in missing documentation for civis_file_to_table
- Include JSON files with pip distributions (#244)
- Added flush to `civis_to_file` when passed a user-created buffer,
ensuring the buffer contains the entire file when subsequently read.

### Added
- Added a utility function which can robustly split a Redshift schema name
Expand Down
1 change: 1 addition & 0 deletions civis/io/_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ def civis_to_file(file_id, buf, api_key=None, client=None):
_civis_to_file(file_id, f, api_key=api_key, client=client)
else:
_civis_to_file(file_id, buf, api_key=api_key, client=client)
buf.flush()
Copy link
Contributor

@mheilman mheilman Mar 19, 2018

Choose a reason for hiding this comment

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

it seems like this should be in _civis_to_file, no?

Copy link
Contributor Author

@waltaskew waltaskew Mar 19, 2018

Choose a reason for hiding this comment

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

it seems like this should be in _civis_to_file, no?

The with open(buf, 'wb') as f: bit ensures the flush happens when the user passes a file path. It's only when they pass a buffer that we need the flush, although it wouldn't hurt to have an extra flush.

Copy link
Contributor

@mheilman mheilman Mar 19, 2018

Choose a reason for hiding this comment

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

should we also rename _civis_to_file to _civis_to_stream or something?

Copy link
Contributor Author

@waltaskew waltaskew Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

should we also rename _civis_to_file to _civis_to_stream or something?

I think of files in the unix sense of anything you can read & write rather than filesystem files, so I don't have an issue with _civis_to_file as a name. I wouldn't object to _civis_to_stream, though.



def _civis_to_file(file_id, buf, api_key=None, client=None):
Expand Down