-
Notifications
You must be signed in to change notification settings - Fork 611
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
Hotfix proper jms message charset #200
base: next
Are you sure you want to change the base?
Hotfix proper jms message charset #200
Conversation
@spindelmanne, @erikryverling, please, review |
Thanks for your contribution! We will review it and get back to you shortly. |
I just noticed you haven't signed our Contributor license agreement yet. You can download it from here: https://dl.dropboxusercontent.com/u/81603711/SmartBear_Contributor_License_Agreement.pdf Could you sign it and send it to us? This is just to let us use your solution in our commercial version without any legal issues. |
@spindelmanne where should I send it? |
You can just email a photo of the signed agreement to manne dot fagerlind at smartbear dot com. |
I think he just did. Replace the dot with . and at with @ |
@spindelmanne I've sent. Can you confirm? |
Hello, finally looking at your PR. I have some issues with you replacing "UTF-8" with StandardCharsets.UTF8. I'm all for doing this (but I would static import it to make the code read better), but if so it should be done consistently in the project. There are currently 519 occurrences of the string "UTF-8", which is obviously technical debt that we should do something about, and you're only changing a couple of them. Do you mind rolling back those changes? As far as I can see, the rest of the PR is good stuff! |
Hey, @privettoli , did you see my comment? |
@spindelmanne, I just was unavailable for these days. Okay, so, as you could see, changes from the hard coded Strings to the StandardCharset constant are in a separate commit, so yeah, we could do revert for these changes. |
@spindelmanne, or I can replace all remaining hardcoded Strings with appropriate constants. |
I prefer to do that as a separate change. It's such a huge code change (although it should not break anything, of course) that I need to do it in collaboration with the SoapUI OS dev team and QA. So just revert the commit for now, OK? |
Use charset from the JMS message or UTF-8 but never use defaultCharset (on Windows it's Win1252) when converting a text to bytes before sending to a broker.
I've found out that bug when our testers created a bug that our app produce symbols like ????? on the places of charset-specific symbols like German and French.