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 UTF-8 charset for setDocument, getDocument #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ZuzooVn
Copy link

@ZuzooVn ZuzooVn commented Oct 23, 2017

According to #17, I have made some modification for setDocument() and getDocument() function.

Please take a look at this pull request and tell me what you think.

Thanks

Nam Vu from ThinkLabs-VN

Copy link
Contributor

@killmenot killmenot left a comment

Choose a reason for hiding this comment

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

@robtweed If you would like to merge this PR I think we also need to add a specific test case that check UTF-8 charset support, WDYT?

@robtweed
Copy link
Owner

My main concern about this suggested change is whether it is compatible with people who don't need UTF-8. If it can be demonstrated that allows getDocument and setDocument to work across all scenarios, then I'll merge it. Agreed it will need specific tests for UTF-8 and non-UTF-8 use-cases

@ZuzooVn
Copy link
Author

ZuzooVn commented Nov 20, 2017

@killmenot @robtweed WDYT?
Any plan to merge this commit?

@ZuzooVn ZuzooVn mentioned this pull request Dec 1, 2017
@killmenot
Copy link
Contributor

@ZuzooVn Yes, we have plan to merge this PR but first we need to complete other work. In the same time, could you please add unit and integration tests for that functionality? We'd like to make sure that UTF-8 and non-UTF-8 works in the same time

@dileep-ehr
Copy link

Will be great if this PR is merged. We are using QEWD-Ripple with Redis and the cache keeps getting corrupted when address contains special characters (eg. Saint-François-de-l'Île-d'Orléans, QC, Canada).

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.

4 participants