ruby / csv

CSV Reading and Writing
https://ruby.github.io/csv/
BSD 2-Clause "Simplified" License
181 stars 114 forks source link

Extract Scanner and InputScanner into external classes #277

Closed fidalgo closed 1 year ago

fidalgo commented 1 year ago

In this commit we extracted the Scanner and InputScanner classes into their own file, reducing the code under the CSV::Parser class.

Deliberately no other change was made, to ensure we do not break compatibility.

kou commented 1 year ago

Why do you want to reduce the code under the CSV::Parser class?

fidalgo commented 1 year ago

Why do you want to reduce the code under the CSV::Parser class?

Making it easier to read and being able to test in isolation.

I stumbled into the CSV code because I was trying initially to see how hard it would be to introduce protections for CSV injection. Looking at the code in CSV class I got scared and thought if I could contribute to making the code more readable and maintainable. One way IMHO to achieve this is to split into more classes with defined responsibilities. This one seems to be low-hanging fruit.

kou commented 1 year ago

Thanks. I understand what you feel.

But it doesn't improve maintainability for me. (I'm a maintainer.) If you're a maintainer too, I respect your feeling. Do you want to be a maintainer in future?

Making it ... being able to test in isolation.

What does it mean? We can test Scanner and InputScanner without this change.

fidalgo commented 1 year ago

@kou I totally understand your arguments, explicitly being the maintainer, which I'm not aspiring to be.

The objective of this PR was to offer a suggestion, by reducing the code in the Parser. Of course, this is a subjective analysis as some prefer isolated classes while others prefer those close to the places they're using them.

So it's up to you if you find any value go ahead if not, just close it (no hard feelings really)

kou commented 1 year ago

OK. Thanks for your suggestion.

I close this for now but we may revisit this again later.