Skip to content

Conversation

@Earlopain
Copy link
Contributor

In ruby 3.5 most of the functionality will require adding cgi as a dependency.

Only escape/unescape functions will remain, which is all that's needed here. cgi/escape is available since Ruby 2.3 with CGI.escapeHTML

https://bugs.ruby-lang.org/issues/21258.

@Earlopain Earlopain changed the title Only require what is needed from cgi Only require what is needed from cgi for Ruby 3.5 compatibility May 9, 2025
@amatsuda
Copy link
Member

amatsuda commented May 9, 2025

@Earlopain Good catch! Thank you!

However, in this case I prefer switching to ERB::Util.html_escape which to me seems to behave exactly the same with (and falls back to) CGI.escapeHTML.

The reasons why I prefer the ERB version are:

  1. We can reduce one line from simplecov-html.rb since we're already requiring 'erb' there. You know, shorter code is always nicer code.
  2. requiring 'cgi/escape' module alone creates an incomplete module that defines broken CGI.unescape method. I know we're not using the unescape method here now, but still, I don't really like that situation.

$ ruby -rcgi/escape -e "CGI.unescape('&')"
-e:1:in 'CGI::Escape#unescape': uninitialized class variable @@accept_charset in #Class:CGI (NameError)

  1. By not directly depending on CGI, we could escape forever from all kinds of CGI fusses and troubles that are supposed to hit us in future versions of Ruby.

In ruby 3.5 most of the functionality will require adding cgi as a dependency.

Only escape/unescape functions will remain, which is all that's needed here. So just use that functionality from `erb`.

https://bugs.ruby-lang.org/issues/21258.
@Earlopain
Copy link
Contributor Author

Your suggestion about using erb for this sounds good to me, I changed it. I didn't test all the methods, so I didn't notice that they don't all just work with this.

@amatsuda amatsuda merged commit 5eebbbb into simplecov-ruby:main May 9, 2025
12 checks passed
@amatsuda
Copy link
Member

amatsuda commented May 9, 2025

@Earlopain Thank you!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants