-
-
Notifications
You must be signed in to change notification settings - Fork 190
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
Switching to using an instance of the SmarterCSV::Reader class #279
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 11 12 +1
Lines 380 435 +55
=========================================
+ Hits 380 435 +55 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall approach looks great. Definitely like just making a distinct new Reader class and wrapping that. I've only been able to skim things so far, but seems like it would fix the threading issues!
I ran your test for a long time, and no thread safety issues. |
@jpcamara @contentfree I'm releasing this as |
hi @tilo thanks for merging this! Is it possible to update the Changelog to make it clear that this gem has become |
thanks for pointing that out! done! |
this PR fixes issues around SmarterCSV being thread-safe, e.g. in response to #277
This PR moves away from using a module method, e.g.
SmarterCSV.process
,and instead using an instance
reader = SmarterCSV::Reader
that contains state, and callingreader.process
on it.For backwards compatibility there is a shim that allows existing code to use
SmarterCSV.process
for the simplest cases, but it no longer gives access to the internal state, e.g. headers, raw_headers, ...Please update your code to the new calling mechanism.