Skip to content
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

Alter CSV and TSV setter methods to return self #323

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

camertron
Copy link

@camertron camertron commented Apr 20, 2019

First of all, thank you for writing and maintaining this excellent library, it's been working flawlessly for us :)

I'm currently in the process of integrating the UniVocity CSV parser into one of our ETL jobs. Here's the code I came up with:

CsvFormat csvFormat = new CsvFormat();
csvFormat.setLineSeparator("/");
csvFormat.setDelimiter(";");

CsvParserSettings csvParserSettings = new CsvParserSettings();
csvParserSettings.setFormat(csvFormat);

CsvParser csvParser = new CsvParser(csvParserSettings);

Whew, that's a lot of code. If all these various setter methods returned this, it would have been a lot more convenient to construct a parser instance:

CsvParser csvParser = new CsvParser(
  new CsvParserSettings().setFormat(
    new CsvFormat()
      .setLineSeparator("/")
      .setDelimiter(";")
  )
)

Ok, so that only saves one line of code (including newlines) 😅 but I would argue it's 1) easier to read, and 2) uses 2 fewer local variables. Plus it could collapse down to fewer lines and only sacrifice readability a little bit:

CsvParser csvParser = new CsvParser(
  new CsvParserSettings().setFormat(
    new CsvFormat().setLineSeparator("/").setDelimiter(";")
  )
)

Anyway, that's what this PR is trying to enable. Thoughts?

@jbax
Copy link
Member

jbax commented Apr 22, 2019

Thanks! I was doing this already for version 3.0.0 (which is a huge refactory) but I'll review your PR in a few days as I'm travelling and make these changes available on version 2.8.2

@camertron
Copy link
Author

@jbax awesome thanks! If you're already working on this then please feel free to close my PR. I only care that it eventually gets done 😄

@travisneeley
Copy link

I would love to see this become part of the current and future code bases for the parsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants