Skip to content

Conversation

@mohitjaggi
Copy link
Contributor

No description provided.

@codecov-io
Copy link

Current coverage is 86.05%

Merging #124 into master will increase coverage by +1.04% as of 928ff17

@@            master    #124   diff @@
======================================
  Files           10      11     +1
  Stmts          407     509   +102
  Branches       125     142    +17
  Methods          0       0       
======================================
+ Hit            346     438    +92
  Partial          0       0       
- Missed          61      71    +10

Review entire Coverage Diff as of 928ff17

Powered by Codecov. Updated on successful CI builds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come comment, schema, and charset are not included in CSVParsingOpts? I don't clearly see a benefit for this level of indirection yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as the parser improves a ton of options are being added. it makes sense to organize them in containers for ease of use. imho, schema is a separate thing. i intended csvparsingopts to contain options that are passed to the csv parsing library. i can change the name if there is a better one you can think of
comment/charset can be here but there was a race between my commit and the one that added it.

@HyukjinKwon
Copy link
Member

@mohitjaggi I think some of them are fixed already and the changes are too big without some descriptions to follow. Wouldn't this be ensible if you close this and maybe submit a PR with smaller one? Or rebase this if you are working on this?

@mohitjaggi mohitjaggi closed this Feb 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants