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

Update to qPython 2.0 and add support for utf-8 #28

Merged
merged 1 commit into from
Jul 28, 2020
Merged

Update to qPython 2.0 and add support for utf-8 #28

merged 1 commit into from
Jul 28, 2020

Conversation

atf1206
Copy link
Contributor

@atf1206 atf1206 commented Jul 13, 2020

This change is focused on updating to qPython 2.0. All code changes below should just be either:

  1. Diffs from current qPython to 2.0;
  2. Add encoding = 'UTF-8' to the qconnection init; and
  3. Patch _write_string to support properly encoding strings to send to kdb. Check out the PR against the qPython repo here: Correctly calculate message length based on encoded length exxeleron/qPython#77

Although this is hopefully straightforward, I recommend testing it for a while before merging since it is such a core part of sublime-q.

Also, a couple things that are NOT changed (yet?):
A. Did not change decoding of the strings in sublime-q, so if you send "õ" you get back "\303\265" (this is how kdb does it, but we could parse it back to "õ" if we wanted).
B. I noticed a small warning,
myLocale=locale.setlocale(category=locale.LC_ALL, locale="en_GB.UTF-8");
File "./python3.3/locale.py", line 573, in setlocale
locale.Error: unsupported locale setting
I believe that since locale is an optional arg, it should just be myLocale=locale.setlocale(category=locale.LC_ALL) but have not included that change here.

@komsit37
Copy link
Owner

This is awesome. I have tested it a bit and all looks good to me so far. I think the change should be ok since most of it is just upgrading qPython.

So, let me know if you are happy with this then I can merge it.

Regarding A) yes, that's how kdb's .Q.s does it. I don't work with non-English so it's not a problem for me. But I imagine it would be helpful if you can display it correctly in sublime text. We can do this in a separate PR if needed.

B) Where do you see this warning? I can't find it in the console.

@atf1206
Copy link
Contributor Author

atf1206 commented Jul 26, 2020

I think it is good as-is. Seems to be close to what you see at the REPL, including:

q)`$"ééé"
`ééé

re: B) It is not a big problem, and in fact I do not expect you to see the error since your machine likely supports en_GB.UTF-8 whereas mine does not. That's why I was suggesting to just let it default to the user's setting. But let's make it a separate PR for the future.

@komsit37 komsit37 merged commit 120ead7 into komsit37:master Jul 28, 2020
@komsit37
Copy link
Owner

Great, I have merged it!

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