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: support wchar_t and Unicode encoding in .list() parameters #63

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

artus9033
Copy link
Contributor

Fixes #62

@reconbot
Copy link
Member

We'd have this problem on OS X too right?

@artus9033
Copy link
Contributor Author

I don't have a way to test that now, but according to this it seems like chars outside of Windows do encode Unicode properly by default, so there should be no need to modify this implementation.

@gcampbell-msft
Copy link

@artus9033 @reconbot @HipsterBrown.

Bringing this up again, is this a valid fix for the issue? If chars outside of Windows do encode Unicode properly by default, that's great, but this fix is for the serialport_win.cpp, right?

I've made a comment here regarding why I'm inquiring about a fix for this. We have a feature that has some issues posted for this and I noticed the bug there that hasn't gotten much traction.

Let me know if there's any way I can help!

@artus9033
Copy link
Contributor Author

@artus9033 @reconbot @HipsterBrown.

Bringing this up again, is this a valid fix for the issue? If chars outside of Windows do encode Unicode properly by default, that's great, but this fix is for the serialport_win.cpp, right?

I've made a comment here regarding why I'm inquiring about a fix for this. We have a feature that has some issues posted for this and I noticed the bug there that hasn't gotten much traction.

Let me know if there's any way I can help!

Indeed, it is the fix you are thinking of.
I believe your comment there is a bit off, as this is not about converting to UTF, but rather converting the code to a data structure that supports UTF. On Windows, char doesn't do that, but wchar_t does.

@reconbot are you going to review and merge this PR?

@gcampbell-msft
Copy link

@artus9033 Thanks for the comment and inquiring about it this is going to be merged. I'll keep an eye out for when this goes in!

@gcampbell-msft
Copy link

@reconbot @artus9033 Re-pinging this in the hope of getting it merged. Thanks!

@artus9033
Copy link
Contributor Author

@reconbot is there any chance that this will be verified and merged? The problem still persists and there has been no progress with this PR in months.

@reconbot
Copy link
Member

I have no way of testing this personally but the code reads well. None of the maintainers have had time to work on serialport right now.

@reconbot reconbot merged commit b0a9aa1 into serialport:main Nov 17, 2022
@reconbot
Copy link
Member

🎉 This PR is included in version 10.7.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants