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 issue #2 #7

Merged
merged 3 commits into from
Dec 7, 2015
Merged

Fix for issue #2 #7

merged 3 commits into from
Dec 7, 2015

Conversation

dhogborg
Copy link
Contributor

@dhogborg dhogborg commented Dec 7, 2015

Fix for issues with Unicode scalar values in normalize function.

fatal error: high- and low-surrogate code points are not valid Unicode scalar values (lldb)

I ran into this and made an attempt to fix it by replacing the String/NSString-mixed implementation with an all String variant

It passes all the tests provided in mstest-0.1/ and has 100% code coverage. All other tests are unaffected. It also works with the markdown that previously crashed it.

@kristopherjohnson
Copy link
Owner

Thanks for this fix. I haven't tested it, but from a quick review it looks like it will no longer strip/convert "\r" values. Not sure how big a deal that is to most users, but I'd like to keep it working the same way.

@dhogborg
Copy link
Contributor Author

dhogborg commented Dec 7, 2015

That was a mistake on my part. There were no \r were part of the test suite (even though there are some in mstest-0.1) and so the conversion was never used and I made the mistake of thinking the carriage returns where handled elsewhere when code coverage reported 0 hits on that block.

I added some tests for \r\r and \r\n to asses how swift handles them and the code is now updated to convert them both to \n\n and \n respectively. As per original implementation.

@kristopherjohnson
Copy link
Owner

Thanks, this is great!

kristopherjohnson added a commit that referenced this pull request Dec 7, 2015
@kristopherjohnson kristopherjohnson merged commit 94ab9b6 into kristopherjohnson:master Dec 7, 2015
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.

2 participants