Skip to content

Avoid ruby 2.4+ methods#2

Closed
kachick wants to merge 1 commit intoruby:masterfrom
kachick:fix-on-ruby2.3
Closed

Avoid ruby 2.4+ methods#2
kachick wants to merge 1 commit intoruby:masterfrom
kachick:fix-on-ruby2.3

Conversation

@kachick
Copy link
Copy Markdown
Member

@kachick kachick commented Mar 25, 2021

matrix:
ruby:
- '2.5'
- '2.6'
- '2.7'
- '3.0'
- head

spec.required_ruby_version = Gem::Requirement.new('>= 2.3.0')

Currently supporting as 2.3+, but testing in 2.5+
This does not run from some reasons.

  • Using String#match?
  • Using String#unpack1

Both are not in ruby 2.3

So I think need some changes.

  • Clarify to support 2.4+ or 2.5+
  • Avoid 2.4+ feature

This PR aims Avoid 2.4+ feature, but some test still fail on rubylang/ruby:2.3.8-bionic even after applied this patch.

Error: test_invalid_trim_mode(TestERBCore): NoMethodError: undefined method `diff' for #<TestERBCore:0x000055c07dff2150>                                                                                         [35/1068]
/usr/src/test/lib/core_assertions.rb:509:in `block in assert_warning'
/usr/src/test/lib/core_assertions.rb:21:in `block in message'
/usr/src/test/lib/core_assertions.rb:510:in `assert_warning'
/usr/src/test/erb/test_erb.rb:239:in `test_invalid_trim_mode'
     236:   end
     237:
     238:   def test_invalid_trim_mode
  => 239:     assert_warning(/#{__FILE__}:#{__LINE__ + 1}/) do
     240:       @erb.new("", trim_mode: 'abc-def')
     241:     end                                                                                                                                                                                                              242:
Failure:                                                                                                                                                                                                         [20/1068]
  pid 18 exit 0
  | warning: -S option of erb command is deprecated. Please do not use this.
  | Passing safe_level with the 2nd argument of ERB.new is deprecated. Do not use it, and specify other arguments as keyword arguments.
  | {:uplevel=>1}
  .

  1. [2/2] Assertion for "stderr"
     | Expected /\n.+\/libexec\/erb:\d+: warning: Passing safe_level with the 2nd argument of ERB\.new is deprecated\. Do not use it, and specify other arguments as keyword arguments\.\n/
     | to match
     |   "\n"+
     |   "Passing safe_level with the 2nd argument of ERB.new is deprecated. Do not use it, and specify other arguments as keyword arguments.\n"+
     |   "{:uplevel=>1}\n"
     | after 1 patterns with 72 characters.
     | <false> is not true.
  <false> is not true.
test_deprecated_option(TestErbCommand)
/usr/src/test/lib/core_assertions.rb:617:in `assert_all_assertions'
/usr/src/test/lib/core_assertions.rb:71:in `assert_in_out_err'
/usr/src/test/erb/test_erb_command.rb:25:in `test_deprecated_option'
     22:       "warning: -S option of erb command is deprecated. Please do not use this.",
     23:       /\n.+\/libexec\/erb:\d+: warning: Passing safe_level with the 2nd argument of ERB\.new is deprecated\. Do not use it, and specify other arguments as keyword arguments\.\n/,
     24:     ]

I'm not sure the root cause.
But this change might be work on Ruby 2.3, Iguess.

I'll create another PR for Clarify to support 2.4+ or 2.5+ .

Still failed in running tests.
@k0kubun
Copy link
Copy Markdown
Member

k0kubun commented Mar 26, 2021

Thanks for providing two different fix patterns. Looks like default gems, in general, seem to respect Ruby's EOL cycles. Since erb.gem has been released very recently and it has been broken, dropping the support should have no practical impact, and this is reversing [Feature 13943] too. So I'd like to accept the other patch #3.

@k0kubun k0kubun closed this Mar 26, 2021
@kachick
Copy link
Copy Markdown
Member Author

kachick commented Mar 26, 2021

Oh I didn't know the existing ruby-lang issue, Thank you for letting me know!

👍 to #3 direction

@kachick kachick deleted the fix-on-ruby2.3 branch March 26, 2021 03:52
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