Add batch support #112
Add batch support #112
Conversation
lib/csv_importer/csv_reader.rb
Outdated
|
|
||
| if use_batches | ||
| io = file_or_content_io | ||
| separator = detect_separator(io.read(1024)) |
There was a problem hiding this comment.
We should extract separator to a method and memoize it as it's called twice.
lib/csv_importer/runner.rb
Outdated
| batch_size: batch_size, | ||
| use_batches: use_batches | ||
| ) | ||
| end |
There was a problem hiding this comment.
Instead of overwriting initialize, what do you think about extracting csv_reader to a method and memoizing it?
lib/csv_importer/csv_reader.rb
Outdated
| end | ||
| end | ||
|
|
||
| def sanitize_content_stream(io) |
There was a problem hiding this comment.
Great method but it's not used. Can you make the Enumerator use it?
|
This is great @ziadsawalha ! I added a couple of comments. Could creating a new class simplify the codebase? e.g. Regarding the public API, We could release this as the next |
|
Thanks for the good feedback. I'll submit updates when I get to it... hopefully tonight (Europe). |
ziadsawalha
force-pushed
the
batching
branch
from
033f9c6 to
2ff239f
Compare
|
I've updated the PR with the following changes:
I struggled with trying to support streams that don't have rewind (which means we need to detect the separator and headers on load and then not try to restart the stream). That complicated things a bit, IMO. More feedback welcome while I try to add more tests and validate this in real life use cases. |
ziadsawalha
force-pushed
the
batching
branch
from
2ff239f to
6ed5a1d
Compare
ziadsawalha
force-pushed
the
batching
branch
from
6ed5a1d to
d37ade1
Compare
|
Great, thank you for making these changes @ziadsawalha ! How does this work against real-life use cases? What kind of streams don't have rewind? S3? Could we close and re-open (or duplicate?!) the stream if it doesn't support rewinding? |
2 participants
Giving this a stab. Feedback welcome!