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

Russian symbols for strurlencode #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Russian symbols for strurlencode #19

wants to merge 2 commits into from

Conversation

NexiusTailer
Copy link

This is a fix that was mentioned in #18, but with all necessary improvements

@oscar-broman
Copy link
Owner

0x255 is 597, I thought this was for an 8-bit character set?

Which character set are used by Russians on Windows? Windows-1251?

@NexiusTailer
Copy link
Author

Now it seems I've made it right and tested.

Which character set are used by Russians on Windows? Windows-1251?

Yes, it was my mistake of choosing wrong symbols table

@NexiusTailer
Copy link
Author

I think it's ready to merge. I've tested it again for sure and it works correctly.

@oscar-broman
Copy link
Owner

One last problem, is that this code is very specific to the russian character set. Not all servers use Windows-1251 so it should not be hardcoded into this.

Looking quickly at RFC-3986, I think the only characters that need encoding are the following:
% ! * ' ( ) ; : @ & = + $ , / ? # [ ]

So it's probably better to make it encode only those. That way, the code does not favour a specific character set and should produce even better URIs (better meaning less %xx).

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