Rewrote code to not use the deprecated URI.escape

[troelskn]

I got some warnings when running tests. This fixes the problem.

[thegengen]

I made the same change locally, but backed out because I didn't have time to test that Google CloudPrint actually supports this kind of encoding.

Have you confirmed that this actually works against Google CloudPrint?

[troelskn]

You mean using + instead of %20? Otherwise the implementation should do the same as before.

[thegengen]

Yes, exactly. Otherwise, I agree, there's no other difference.

[troelskn]

Well, it's correct according to specs, but I suppose that isn't a guarantee that it will work in practise. Not entirely sure how to test it though.

To be safe, I could change the code to gsub + to %20. It should still be valid and then we have full backwards compatibility.

[thegengen]

Let me try this out tomorrow. I don't think gsub is how we want to go, there could be other symbols that are encoded differently, it feels a bit like opening Pandora's box.

I'll get back to you as soon as I know whether this works or not.

[thegengen]

OK. So I've tried this out locally, looked at your code again and have a couple of observations.

• I don't think there's a need to merge the URI query with the params. The URI query should not have any params in it when this code gets called.
• The form encoding does indeed work so the gsub is not needed.
• This code is old and crufty and this should just be the start of cleaning it up. In particular, oauth2 brings faraday in as a dependency, so we might as well be using it as well going forward.

That leaves me with this code (for now):

    def build_get_uri(uri, params = {})
      unescaped_params = params.map { |key,val| [key, val] }
      escaped_params = URI.encode_www_form(unescaped_params)

      escaped_params = "?#{escaped_params}" unless escaped_params.empty?
      uri + escaped_params
    end

Anything about this code that wouldn't work for you? If you think it works, feel free to change your PR. Alternately, I can make the change myself.

[troelskn]

I don't think there's a need to merge the URI query with the params. The URI query should not have any params in it when this code gets called.

I couldn't tell from the code if that was the case, but sounds right.

The form encoding does indeed work so the gsub is not needed.

It would've been surprising otherwise, but cool that you checked.

This code is old and crufty and this should just be the start of cleaning it up. In particular, oauth2 brings faraday in as a dependency, so we might as well be using it as well going forward.

Very much agree. It would also be cool to include helper methods for the authentication process, rather than just linking to docs. Also, right now there is no obvious way of reusing a valid access token - the lib will have to re-negotiate from the refresh-token for each instance.

I'll make the changes as suggested and rebase, so we can get this one out of the way. I think I can find some time to help with rewriting to use Faraday, but no promises though.

[thegengen]

Yes, checking that the escape works reminded me just how hard following that process by hand is. 😓 I plan to push some updates for the authentication process next week. I'm going to create an issue for the Faraday change, do drop a note if you find the time to tackle it next week.

Thanks so much for your help so far!