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: Do not remove space after comma in string value #37

Merged
merged 9 commits into from Oct 9, 2019

Conversation

@localheinz
Copy link
Member

commented Dec 28, 2017

This PR

  • asserts that a space after a , in string value is left alone
  • leaves the space after a , in a string value alone

Fixes #2.

πŸ’β€β™‚οΈ Not sure if a fix yet, but figured I provide a failing test at least.

@localheinz localheinz force-pushed the localheinz:fix/string-with-comma branch 2 times, most recently from 95b9d28 to 33f4f33 Jan 3, 2018
@weierophinney weierophinney force-pushed the localheinz:fix/string-with-comma branch from 8f83f85 to 1ba1274 Jan 4, 2018
@weierophinney

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

@localheinz I've rebased it off of current master, and the tests demonstrate the issue.

I've no idea how to fix it presently; carry on if you want, or ping me or others for assistance if you need to.

@weierophinney weierophinney force-pushed the localheinz:fix/string-with-comma branch from 1ba1274 to e80fad8 Jan 4, 2018
webimpress added 2 commits Oct 7, 2019
Ignores commas and other special characters in string values
@webimpress

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

@localheinz @weierophinney
I've resolved the issue. First I've removed duplicated tests - somehow after @weierophinney rebase some tests were added twice. Then I've changed completely approach in prettyPrint so all tests are now passing. Added more tests to check the behaviour for other edge cases.

Let me know what you think, and if you ok with it, I'll include it in the next minor release. Thanks!

@webimpress webimpress requested review from froschdesign and weierophinney Oct 7, 2019
@webimpress webimpress added the bug label Oct 7, 2019
@webimpress webimpress added this to the 3.1.2 milestone Oct 7, 2019
webimpress added 3 commits Oct 7, 2019
Copy link
Member

left a comment

Solution looks good - I have some suggestions for readability below.

src/Json.php Outdated Show resolved Hide resolved
src/Json.php Outdated Show resolved Hide resolved
webimpress added 2 commits Oct 8, 2019
Copy link
Member

left a comment

🚒

webimpress added a commit that referenced this pull request Oct 9, 2019
Fix: Do not remove space after comma in string value
webimpress added a commit that referenced this pull request Oct 9, 2019
@webimpress webimpress merged commit baaa4b1 into zendframework:master Oct 9, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@webimpress

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Thanks, @localheinz!

webimpress added a commit that referenced this pull request Oct 9, 2019
Forward port #37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.