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

do not lie about the strings encoding #921

Closed
wants to merge 12 commits into from

Conversation

james-lawrence
Copy link

one possible way to address #902
this is more of pointing out the issue further than an actual proposed fix.

@jeremy
Copy link
Collaborator

jeremy commented Oct 20, 2015

Think this is a good idea. Pragmatic choice.

@bf4
Copy link
Collaborator

bf4 commented Oct 21, 2015

👍 Meanwhile, in terms of a fix, I covered all encoding cases in rspec-support

Perhaps we/I should extract parts of that

@james-lawrence
Copy link
Author

have some further tests/fixes. this code is broken when emails contain array header fields with multiple encodings. the gsub fails and blows up wonderfully if a string is say utf-7 encoded.

changes needed + test coming up as well

@james-lawrence
Copy link
Author

further investigation leads me to say that the multiple encodings isn't a problem unless ruby can't correctly decode once of the encoding, at which point it should blow up.

the change to the collapse method was mainly because it was generating a weird array, sometimes incorrectly parsing the encoded values. so I stripped out the regex. Anyways collapse method is likely just for our internal use atm until the underlying problem is fixed (i.e correctly blows up on strings that can't be decoded -> ::Encoding::UTF_7, ::Encoding::CP1252)

@bf4
Copy link
Collaborator

bf4 commented Oct 26, 2015

Extracted https://github.com/bf4/encoded_string with rspec's blessing.. It's well-tested, but I haven't done much with it on its own. Feel free to try it out

@james-lawrence
Copy link
Author

dead pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants