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

Fix for incorrect handling of embedded quotes when printing from Read… #78

Closed

Conversation

arcticgeek
Copy link

@arcticgeek arcticgeek commented Jul 20, 2020

Fix for incorrect handling of embedded quotes when printing from Reader and to related test; added new test class to further exercise CSVFormat.printWithQuotes(Reader,Appendable) for expected behavior. CSV-263

…er and to related test; added new test class to further exercise CSVFormat.printWithQuotes(Reader,Appendable) for expected behavior.
@coveralls
Copy link

coveralls commented Jul 20, 2020

Coverage Status

Coverage remained the same at 98.506% when pulling 500a3b0 on arcticgeek:fix-print-quoted-from-reader into c5bd432 on apache:master.

Reader and to related test; added new test class to further exercise
CSVFormat.printWithQuotes(Reader,Appendable) for expected behavior.
@garydgregory
Copy link
Member

May you please rebase on master?

@arcticgeek
Copy link
Author

arcticgeek commented Jul 7, 2021

Hi Gary, I tried this morning to do what you have reasonably asked of me. While trying "rebase my patch" on master, I think what I've done now is merged all of the changes which have occurred in master since my original repository fork into my patch fork instead of committing just my changes updated relative to what is now on master. And I don't know how to fix it so that my patch is nice and clean like I took the time to do originally a year ago.

My patch in this pull request is an important fix that I would like to see merged so I can stop working around the bug. I understand abstractly what you want me to do, and making the code changes was easy enough, but I just don't understand git well enough to satisfy your ask. I'm willing to try to fix this with your guidance but otherwise I don't know how to proceed and I'm sorry about that.

@garydgregory
Copy link
Member

@arcticgeek
Just look at the Files tab in GitHub and check that the changes are what you want.

Copy link
Author

@arcticgeek arcticgeek left a comment

Choose a reason for hiding this comment

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

I have carefully looked at the changes again, marked all the files as viewed, and then submitted this comment in review of the pull. This simple refactoring of CSVFormat.printWithQuotes(Reader, Appendable) addresses the embedding issue and the included test case is valid for exercising this method to ensure correct behavior in the future.

@garydgregory
Copy link
Member

@arcticgeek
The build fails. Please see the GitHub logs.

Copy link
Author

@arcticgeek arcticgeek left a comment

Choose a reason for hiding this comment

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

The original test case CSVFormatTest.testPrintWithQuotes() was confirming the incorrect behavior and somehow I forgot to include my adjustment to it with my rebase commit.

This fix to the original test case is what prompted me to further exercise the CSVFormat.printWithQuotes() method with additional tests which are found in the JiraCsv263Test class.

@garydgregory
Copy link
Member

@garydgregory
Copy link
Member

@arcticgeek
OK, looks good, I'll resolve the conflicts I just made in master and merge.

asfgit pushed a commit that referenced this pull request Jul 7, 2021
output.

- Resolve conflicts from PR #78 by Jason A. Guild.
- Don't use depreacted methods.
- Javadoc.
- Use final.
@garydgregory
Copy link
Member

Closing, in git master now.

@arcticgeek arcticgeek deleted the fix-print-quoted-from-reader branch July 7, 2021 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants