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

Respect Regexp.compile method signature #9351

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented May 18, 2024

As far as I can tell, Regexp.compile has never accepted a third parameter to Regexp.compile until Ruby 3.2. There it actually is a timeout parameter. In older versions it was discarded as an invalid input. It's unclear to me if this parameter ever worked in the first place.

This PR attemps to still make it work as it was originally intended, but it's unclear what the original goal even was. Perhaps it's better to make an incompatible change to drop the third parameter and instead enhance the flags parameter to support N as a value.

@ekohl ekohl requested a review from a team as a code owner May 18, 2024 16:44
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@joshcooper
Copy link
Contributor

I believe Regexp.compile accepted a third argument in Ruby 1.8, but in 1.9 it was changed to always ignore it ruby/ruby@98e6f9a I'm thinking we should do something similar:

if encoding
  Puppet.warn_once(
    'deprecations', 'regsubst_function_encoding',
    _("The regsubst() function's encoding argument has been ignored since Ruby 1.9 and will be removed in a future release")
  )
end

Since Ruby 1.9 the encoding argument to Regexp.compile has been ignored
and has been removed since. Starting Ruby 3.2 there is a new third
positional argument for the timeout.

This PR deprecates the old method with encoding and stops passing the
argument, making it a dead parameter.
@ekohl
Copy link
Contributor Author

ekohl commented May 28, 2024

So something like this?

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.

None yet

3 participants