Closed rbmrclo closed 4 years ago
Seems fine to get code working in 2.5. Does 2.5 not have the same performance issue?
@jpmckinney Thanks for the review!
Does 2.5 not have the same performance issue?
Unfortunately, I haven't gone deeper on that. Ideally, if we started to update csvlint and support 2.5
, optimisation should be something that we would want to revisit again, especially on doing the same amazing improvements that you did here. ☺️
For now, the purpose of this band-aid fix is to just really keep the ball rolling on 2.5
.
This bug is preventing us from upgrading some of our apps to Ruby 2.5 😞 Taking a quick look it looks like this optimization still makes sense... but this just looks terrible:
# Optimization: Disable the CSV library's converters feature.
# @see https://github.com/ruby/ruby/blob/v2_2_3/lib/csv.rb#L2100
if RUBY_VERSION < '2.5'
def init_converters(options, field_name = :converters)
@converters = []
@header_converters = []
options.delete(:unconverted_fields)
options.delete(field_name)
end
else
def init_converters(converters, ivar_name, convert_method)
instance_variable_set(ivar_name, [])
end
end
Currently, when using csvlint on ruby 2.5.0, all validations are raising exceptions with :invalid_encoding message. When it's not really the cause of the issue.
Spent several hours trying to work out why this was happening, thinking that there was something wrong with the csv; thanks for highlighting the issue.
Hi @theodi, @Floppy, @quadrophobiac,
Is there any concern stopping this PR from being merged?
The latest version of Ruby is now 2.6.1
but this bug keeps some users at 2.4.*
. 😭
Update: This bug only exists on Ruby 2.5.
Hi @Floppy @quadrophobiac, thanks for this amazing library, but can you let us know when this PR would be merged or is there anything else required?
Thank you!
Neither @quadrophobiac or I have push access to this any more as we’re not at ODI any more. But, it doesn’t look like anyone is watching over there. I will see if I can chase someone down and get this into a state where we can get some new maintainers on. I’m happy to be one of them...
Thank you @Floppy. And, getting this gem into an active state would be great as it provides so many cool features.
Hi folks, Can someone approve this merge please and make a new release? The merging is blocked at the moment. We are having the same issue!
Good news! csvlint has been adopted by the Data Liberation Front (a few of us who wrote it originally, now ex-ODI), so I can merge this in! I agree that the optimisation can be revisited separately for ruby >= 2.5
This is awesome, thank you @Floppy !
Background
csvlint
library patched the methodCSV#init_converters
2.5.0
2.5.0
, the required arguments are increased to threeLineCSV
will fallback and use the default#init_converters
method ofCSV
for ruby2.5.0
and above.csvlint
onruby 2.5.0
, all validations are raising exceptions with:invalid_encoding
message. When it's not really the cause of the issue.Please review and let me know your thoughts!
cc @jpmckinney