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

Added CSV change to CDL.java #219

Merged
merged 1 commit into from
May 2, 2016
Merged

Conversation

brianrussell2
Copy link
Contributor

Adding my code for the GitHub issue I described.

@stleary
Copy link
Owner

stleary commented Apr 13, 2016

Looks reasonable. Need to run it through the unit tests, and add some cases to exercise the new code. @captainIowa you are welcome to take the lead on that if you want. The unit tests are located in https://github.com/stleary/JSON-Java-unit-test. Otherwise I will take care of it when I get a chance.

@brianrussell2
Copy link
Contributor Author

@stleary I can take a run at it this weekend.

@brianrussell2
Copy link
Contributor Author

@stleary I ran my changes through the unit tests and added some additional ones to exercise my code changes. Do I need to post the results of the unit tests somewhere or take any additional steps?

@stleary
Copy link
Owner

stleary commented Apr 25, 2016

@captainIowa thanks, no other actions are needed, reviewers will take a look and let you know if there are any concerns.

@johnjaylward
Copy link
Contributor

The change looks good to me upon review. I did not check out and run the code myself though. As long as all the new tests pass, I think this is good to go.

@stleary
Copy link
Owner

stleary commented Apr 27, 2016

What problem does this code solve?
Support in the CDL module for parsing embedded quotes in CSV files to maintain compatibility with RFC 4180. For additional discussion, see #217.

Changes to the API?
No.

Does it break the unit tests?
No, but new unit tests will be added after this commit. Those tests will fail unless this code is committed. See stleary/JSON-Java-unit-test#45.

Will this require a new release?
No, this will be rolled into the next release, which is not yet scheduled.

Should the documentation be updated?
No, this is an internal bug fix to maintain compatibility with the spec.

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.

None yet

3 participants