Fix broken encoding (fixes #920)

[lparry]

We have run into problems where using client.content() returns an 8-bit ascii string, while the source file is UTF-8, as is reaffirmed by the charset within the content type header.

This PR checks for that charset header, and when found calls #force_encoding on the ascii string to make the UTF-8 characters be interpreted correctly.

I had to do some VCR cassette doctoring in order to reference the new fixture as though it was already in the canonical repo and already on master. Hopefully I didn't mess it up in some unforeseen way

Should fix #920

[lparry]

Please let me know if there's anything I can do to help get this merged. Thanks!

[daniel-beard]

Am also hitting this issue, is there any path to merging this PR?

[lparry]

:tumbleweeds:

[CarlaBehan1212]

Thanks for your help with this matter is greatly appreciated and I will get it from the working class people who have been trying to get in touch with me and well-being.

[eneko]

Please fix. We've been having this issue for a long time, as Danger relies on this library for diffing pull requests.

[lparry]

Hey @hmharvey @tarebyte, I see two releases have happened since this was opened, but this fix has not been included.

Can you please let me know what needs to happen to make this PR acceptable to be merged, so that our team can stop using our own fork?

[tarebyte]

@lparry apologies for the delay. After conferring with a few colleagues to verify my initial reaction, we've come to the conclusion that this is a breaking change. We're working toward the release of v5 but I think it'll be a while before we get there with the generated client.

~~I'm still not entirely sure why this is always returning 8-bit ascii string so I will probably need to track that down. Perhaps it's something to do with the Sawyer/Faraday combo.~~

It's an issue with the adapter that is used with Faraday https://github.com/lostisland/faraday/issues/139. Some adapters handle it better than others.

In the mean time we can still work together to make this change more robust for all of our use cases. Here's probably what we'll need to do:

• [ ] Reach out to GitHub Support and find out all of the various types of formats that are sent back.
• [ ] Add tests covering all of those cases (and solve the Base64 case as you mentioned)
• [ ] Add a warning to let folks know that the response format will be changing in a future version.

If this sounds agreeable please let me know @lparry, and again thanks for being so patient.

[tarebyte]

@lparry thinking about this more, this sounds like a good middleware that we could allow users to opt into. I don't think that would be a breaking change.

[titanium-cranium]

Hi @tarebyte: Unfortunately, @lparry has recently moved on to another company and I'm now picking up the ball to determine the status of the proposed fix. Any progress?

[tarebyte]

I'm now picking up the ball to determine the status of the proposed fix. Any progress?

I haven't at this point. I might try and look at it in the next few weeks. It's GitHub Satellite this week and I'm on vacation the next.

[titanium-cranium]

I haven't at this point. I might try and look at it in the next few weeks. It's GitHub Satellite this week and I'm on vacation the next.

Beauty! Many thanks. 🙏

[lucascaton]

Hi @tarebyte! Unfortunately, @titanium-cranium has recently moved on to another company and I'm now picking up the ball to determine the status of the proposed fix. #dejavu 😅

Is there any news on this? Could you please review and merge this change? Thank you!

[nickfloyd]

'👋 Hey Friends, this pull request has been marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: pinned label if this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!'

[lucascaton]

Hey @nickfloyd! 👋 Why not merge it, instead?

We've been using a fork with this solution and it's been working well since 2019 (when this PR was opened).

[nickfloyd]

Hey @lucascaton, thanks for the response; we can do that! Being over a year old with merge conflicts will make that a bit challenging if we do not have push access to envato:lparry/fix-broken-encoding. Would you be interested in resolving those conflicts so we can give this a proper review and potentially get it merged in?

If not, no worries; we'll prioritize it and see if we can reach out to @lparry to enable edits for maintainers if needed, if not we can hoist these changes into another more recent branch of main and get it in that way.

[lucascaton]

Hi @nickfloyd, I've just merged main into this branch, it should be all good to review now 🙂 Thanks!

[lucascaton]

Hi @nickfloyd, is there anything else you'd like me to change? Thanks!