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

Fix positional argument deprecations with ERB.new #23250

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Oct 30, 2024

Extracted from #23225

Drop second argument as safe_level was removed.
Change third argument for trim_mode to keyword argument.

Oops, this PR doesn't need cross repo. 😅

Drop second argument as safe_level was removed.
Change third argument for trim_mode to keyword argument.
@jrafanie jrafanie requested a review from Fryguy as a code owner October 30, 2024 18:30
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Oct 30, 2024
@jrafanie jrafanie changed the title [WIP] Fix positional argument deprecations with ERB.new Fix positional argument deprecations with ERB.new Oct 30, 2024
@jrafanie jrafanie mentioned this pull request Oct 30, 2024
40 tasks
@Fryguy
Copy link
Member

Fryguy commented Oct 30, 2024

@jrafanie I thought we couldn't do this PR cause it doesn't work on Ruby 3.0 (I'm surprised tests are passing, which tells me we're not testing this path?) This is similar to ManageIQ/manageiq-appliance_console#267

@jrafanie
Copy link
Member Author

jrafanie commented Oct 30, 2024

@jrafanie I thought we couldn't do this PR cause it doesn't work on Ruby 3.0 (I'm surprised tests are passing, which tells me we're not testing this path?) This is similar to ManageIQ/manageiq-appliance_console#267

hmm, I thought it sounded familiar. I'll have to take a look. The deprecation was certainly raised by the test suite as that's how I found it. Perhaps the test is exercising the code but somehow still passing. Let me see if I can see what erb is being used in the passing 3.0 tests.

EDIT: See below... the test was exercising it correctly and it looks like ERB accepts either trim_mode positional or kwarg since ruby 2.6. Safe level can be removed safely.

@@ -42,7 +42,7 @@ def create
LOGGER.debug("Invoked #{self.class}##{__method__}")

begin
File.write(dest_path, ERB.new(File.read(src_path), nil, '-').result(binding))
File.write(dest_path, ERB.new(File.read(src_path), :trim_mode => '-').result(binding))
Copy link
Member Author

@jrafanie jrafanie Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, ruby 3.0.7 has 2.2.0 which has this interface:

  def initialize(str, safe_level=NOT_GIVEN, legacy_trim_mode=NOT_GIVEN, legacy_eoutvar=NOT_GIVEN, trim_mode: nil, eoutvar: '_erbout')

I dropped the safe_level and passed a trim_mode kwarg so it looks like it's a bridge interface to allow legacy interface with positional arguments so what I did above still works.

Note, ruby 3.0.0 has the same erb interface.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@jrafanie jrafanie Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I am good with ManageIQ/manageiq-appliance_console#267. It looks like the interface was changed in ruby 2.6 to accept either kwargs or positional arg for trim_level. Safe level can be excluded safely

EDIT: see ruby/ruby@cc777d0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I;m so lost - when I researched this the other day, I could have sworn that 2.2.0 (from Ruby 3.0) didn't have the keyword args interface.

@Fryguy
Copy link
Member

Fryguy commented Oct 30, 2024

The Gemfile.lock.release on radjabov doesn't show erb directly, which tells me it's the default gem which is 2.2.0, which doesn't have the keyword args. which does have the keyword args - seems I was completely wrong 😄

@Fryguy
Copy link
Member

Fryguy commented Oct 30, 2024

Actually, radjabov is 3.1, I think, so maybe it doesn't matter except for quinteros?

EDIT: I typo'd above and it confused me - updating the previous comment to be more accurate.

@Fryguy
Copy link
Member

Fryguy commented Oct 30, 2024

Removing the Rails 7.1 label cause it really isn't related - just happened to be found during that investigation

@Fryguy Fryguy merged commit 228fdbc into ManageIQ:master Oct 30, 2024
12 checks passed
@Fryguy Fryguy self-assigned this Oct 30, 2024
@jrafanie jrafanie deleted the fix_erb_new_deprecations branch October 30, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants