add ruby 3.1 and 3.2 support, drop ruby 2.6 #355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
razumau
force-pushed
the
jury.razumau/ruby_31_32
branch
from
c68a2a8 to
b647c71
Compare
razumau
force-pushed
the
jury.razumau/ruby_31_32
branch
from
219f3cb to
37cc50c
Compare
razumau
commented
May 16, 2023
| @@ -434,7 +434,8 @@ def no_template_format_error(manifest) | |||
|
|
|||
| def valid_url?(value) | |||
| uri = URI.parse(value) | |||
| uri.is_a?(URI::HTTP) && !uri.host.nil? | |||
| host_empty = uri.host.nil? || uri.host == '' | |||
| uri.is_a?(URI::HTTP) && !host_empty | |||
There was a problem hiding this comment.
In newer URI versions, host for "https://" is an empty string, not nil.
razumau
commented
May 16, 2023
| @@ -18,7 +18,7 @@ def message | |||
|
|
|||
| # if the error contains the word `_legacy` in the second sentence, let's | |||
| # only use the first one. | |||
| if [original, attempted].any? { |val| val =~ /_legacy/ } | |||
| if [original, attempted].any? { |val| val.is_a?(String) && val =~ /_legacy/ } | |||
There was a problem hiding this comment.
=~ for non-strings has been removed from recent Ruby versions (and was deprecated—and often meaningless—before), so now we compare only strings to this regex.
razumau
commented
May 16, 2023
| @@ -300,7 +300,7 @@ def build_app_source_with_files(files) | |||
|
|
|||
| it 'includes zh-cn in translations' do | |||
| expected_translations = JSON.parse(File.read('spec/translations/zh-cn.json')) | |||
| expect(package.send(:translations)['zh-cn'].except('custom1')).to eq(expected_translations) | |||
| expect(I18n::Utils.except(package.send(:translations)['zh-cn'], 'custom1')).to eq(expected_translations) | |||
There was a problem hiding this comment.
Hash refinements are removed in recents i18n versions (ruby-i18n/i18n#573).
razumau
commented
May 16, 2023
| @@ -319,7 +319,7 @@ def build_app_source_with_files(files) | |||
|
|
|||
| it 'removes zendesk-specific keys in translations' do | |||
| expected_translations = JSON.parse(File.read('spec/translations/zh-cn.json')) | |||
| expect(package.send(:translations)['zh-cn'].except('custom1')).to eq(expected_translations) | |||
| expect(I18n::Utils.except(package.send(:translations)['zh-cn'], 'custom1')).to eq(expected_translations) | |||
There was a problem hiding this comment.
Hash refinements are removed in recents i18n versions (ruby-i18n/i18n#573).
Newer URI versions set host to an empty string (instead of nil) for URL like “https://”
razumau
force-pushed
the
jury.razumau/ruby_31_32
branch
from
37cc50c to
847c967
Compare
bquorning
reviewed
May 16, 2023
| @@ -18,7 +18,7 @@ def message | |||
|
|
|||
| # if the error contains the word `_legacy` in the second sentence, let's | |||
| # only use the first one. | |||
| if [original, attempted].any? { |val| val =~ /_legacy/ } | |||
| if [original, attempted].any? { |val| val.is_a?(String) && val =~ /_legacy/ } | |||
There was a problem hiding this comment.
Would the following work?
Suggested change
| if [original, attempted].any? { |val| val.is_a?(String) && val =~ /_legacy/ } | |
| if [original, attempted].any? { |val| /_legacy/.match?(val) } |
There was a problem hiding this comment.
No: val is sometimes (at least in tests) a Hash (of all things), so that also fails with TypeError: no implicit conversion of Hash into String.
Shengming-Yan
approved these changes
May 31, 2023
mmassaki
approved these changes
Jun 2, 2023
| s.license = 'Apache License Version 2.0' | ||
| s.authors = ['James A. Rosen', 'Likun Liu', 'Sean Caffery', 'Daniel Ribeiro'] | ||
| s.email = ['[email protected]'] | ||
| s.homepage = 'http://github.com/zendesk/zendesk_apps_support' | ||
| s.summary = 'Support to help you develop Zendesk Apps.' | ||
| s.description = s.summary | ||
|
|
||
| s.required_ruby_version = Gem::Requirement.new('>= 2.6', '< 3.1') | ||
| s.required_ruby_version = Gem::Requirement.new('>= 2.7') |
There was a problem hiding this comment.
I think we can drop v2.7 support given it's past EOL now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds Ruby 3.1 and 3.2 to the test matrix, drops Ruby 2.6, updates tests dependencies (to versions with support for recent Ruby versions).
This required a couple of changes:
— keyword arguments in i18n’s
translatemethod (because of that, I’ve added a lower limit for i18n’s version in the gemspec—this release);— Hash refinements are removed in recents i18n versions (ruby-i18n/i18n#573), so instead we use
I18n::Utils.exceptin tests now;— In newer
URIversions, host for "https://" is an empty string, not nil, so we check for both.I’ve also cleaned up tests workflow and added a
tests_successfulwrapper.Before merging this PR
Risks