Changes to calls to `escape`, `unescape` and `open`

[eddierubeiz]

To ease the transition to Rails 3.0, I'm making the following changes:

• URI.escape → CGI.escape
• URI.unescape → CGI.unescape
• open(some_url) → URI.open(some_url)

[jrochkind]

I'm not totally sure CGI.escape/unescape are suitable subsitutes, they definitely don't have the exact same semantics... this has been a confusing thing for me in rubydom generally.

And I don't understand the desired semantics or possible implications or backwards incompatibilities where they are used here in qa.

So I'd love some feedback from other samverites. @cjcolvar ? @jrgriffiniii ?

NOTE: URI.escape/CGI.escape don't actually appear in the diff for PR, only the "unescape" side is here.

[eddierubeiz]

Oh, check it out: https://github.com/samvera/questioning_authority/pull/210 proposes ERB::Util.url_encode as the replacement for URI.escape (which indeed does not occur anywhere in the code after that PR).

[eddierubeiz]

(Setting this one to draft until I dig a bit more.)

[jrochkind]

Weird, #210 says it was merged! Why aren't it's changes in the main branch, already taking care of this three years ago?

[jrochkind]

Ah right, because this PR is just for unescape, not escape!

I don't know if ERB::Util has an unescape equiv.

This is one of my least favorite things about ruby what they did to us with URI.escape.

[jrochkind]

OK, I really don't understand what URI.unescape is doing in this code in the first place... it doesn't make any sense to me, I wonder if it's either a bug or a mis-specified spec in the first place... what's it supposed to be doing and why?

If we just skip the unescape step, do all tests still pass? If any tests fail, it might at least give us a clue as to what the use case is.

We might need some advice from other samverians here, especially if we can find anyone using these particular qa adapters. (We do not). @rotated8 , @cjcolvar , @jrgriffiniii ?

[jrochkind]

So tests all pass with calls to unescape simply removed.

I don't understand the purpose or specifications leading to the unescape calls. Why would id need unescaping, isn't it just a bug somewhere if the variable holds an escaped form already?

My proposal is to remove "unescape" altogether. I think we should try to get feedback from other samverians (perhaps at next week's tech meeting?), and if nobody has the use case/specs here, simply remove the unescape -- if it causes a problem for someone, then when they let us know we'll be closer to understanding use case and specifications that require an unescape, and can add it back with docs and tests.

[eddierubeiz]

Closing this as superseded by #367 and #368 .