-
-
Notifications
You must be signed in to change notification settings - Fork 11
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 frozen string literal issue for ruby 3.4 #27
Merged
+170
−10
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,5 +12,21 @@ permissions: | |
contents: read | ||
|
||
jobs: | ||
Shared: | ||
uses: fog/.github/.github/workflows/[email protected] | ||
test: | ||
|
||
runs-on: ubuntu-latest | ||
|
||
strategy: | ||
matrix: | ||
ruby-version: ['3.0', '3.1', '3.2', '3.3', 'head'] | ||
continue-on-error: ${{ matrix.ruby-version == 'head' }} | ||
|
||
steps: | ||
- uses: actions/checkout@v4 | ||
- name: Set up Ruby | ||
uses: ruby/setup-ruby@v1 | ||
with: | ||
ruby-version: ${{ matrix.ruby-version }} | ||
bundler-cache: true # runs 'bundle install' and caches installed gems automatically | ||
- name: Run tests | ||
run: bundle exec rake RUBYOPT="--enable-frozen-string-literal" |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ def reset | |
end | ||
|
||
def characters(string) | ||
@value ||= '' | ||
@value ||= +'' | ||
@value << string | ||
end | ||
|
||
|
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,20 +5,22 @@ def initialize | |
end | ||
|
||
def characters(string) | ||
@value ||= "" | ||
@value ||= +'' | ||
@value << string.strip | ||
end | ||
|
||
def end_element(name) | ||
last = @stack.pop | ||
if last.empty? && @value.empty? | ||
@stack.last[name.to_sym] = "" | ||
@stack.push({}) if @stack.empty? | ||
geemus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if last&.empty? && @value.empty? | ||
@stack.last[name.to_sym] = +'' | ||
elsif last == { :i_nil => "true" } | ||
@stack.last[name.to_sym] = nil | ||
elsif [email protected]? | ||
@stack.last[name.to_sym] = @value | ||
end | ||
@value = "" | ||
@value = +'' | ||
end | ||
|
||
def body | ||
|
@@ -30,7 +32,7 @@ def response | |
end | ||
|
||
def start_element(name, attributes = []) | ||
@value = "" | ||
@value = +'' | ||
parsed_attributes = {} | ||
until attributes.empty? | ||
if attributes.first.is_a?(Array) | ||
|
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'minitest_helper' | ||
chaadow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
require 'fog/xml' | ||
|
||
# We expose accessors just for testing purposes | ||
Fog::ToHashDocument.attr_accessor(:value, :stack) | ||
|
||
describe Fog::ToHashDocument do | ||
chaadow marked this conversation as resolved.
Show resolved
Hide resolved
chaadow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
before do | ||
@document = Fog::ToHashDocument.new | ||
end | ||
|
||
describe '#characters' do | ||
it 'appends characters to @value' do | ||
@document.characters('some text') | ||
_(@document.value).must_equal 'some text' | ||
end | ||
|
||
it 'strips whitespace from characters' do | ||
@document.characters(' some text ') | ||
_(@document.value).must_equal 'some text' | ||
end | ||
end | ||
|
||
describe '#end_element' do | ||
chaadow marked this conversation as resolved.
Show resolved
Hide resolved
chaadow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
before do | ||
@document.stack << {} | ||
@document.characters('some text') | ||
end | ||
|
||
it 'adds element with text content to the stack' do | ||
@document.end_element('element') | ||
|
||
expected = { element: 'some text' } | ||
_(@document.stack.last).must_equal(expected) | ||
end | ||
|
||
it 'can mutate the new empty value' do | ||
@document.end_element('element') | ||
|
||
_(@document.value).must_equal('') | ||
|
||
# Mutate the new empty value even when frozen string literals are enabled | ||
_(@document.characters('one')) | ||
end | ||
|
||
it 'adds empty string if element is empty and value is empty' do | ||
@document.value = '' | ||
|
||
@document.end_element('element') | ||
|
||
expected = { element: '' } | ||
_(@document.stack.last).must_equal(expected) | ||
end | ||
|
||
it 'adds nil if element has :i_nil attribute' do | ||
@document.stack.last[:i_nil] = 'true' | ||
@document.value = '' | ||
|
||
@document.end_element('element') | ||
|
||
expected = { element: nil } | ||
_(@document.stack.last).must_equal(expected) | ||
end | ||
end | ||
|
||
describe '#body' do | ||
it 'returns the first element of the stack' do | ||
@document.stack << { key: 'value' } | ||
|
||
expected = { key: 'value' } | ||
_(@document.body).must_equal(expected) | ||
end | ||
end | ||
|
||
describe '#response' do | ||
it 'returns the body' do | ||
@document.stack << { key: 'value' } | ||
|
||
expected = { key: 'value' } | ||
_(@document.response).must_equal(expected) | ||
end | ||
end | ||
|
||
describe '#start_element' do | ||
chaadow marked this conversation as resolved.
Show resolved
Hide resolved
chaadow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
it 'parses attributes correctly' do | ||
@document.start_element('element', [%w[key value]]) | ||
|
||
expected = { key: 'value' } | ||
_(@document.stack.last).must_equal(expected) | ||
end | ||
|
||
it 'handles elements without attributes' do | ||
@document.start_element('element') | ||
|
||
_(@document.stack.last).must_equal({}) | ||
end | ||
|
||
it 'adds nested elements to the stack' do | ||
@document.start_element('parent') | ||
@document.start_element('child') | ||
|
||
_(@document.stack).must_equal([{ child: {} }, { child: {} }, {}]) | ||
end | ||
|
||
it 'adds nested elements with attributes to the stack' do | ||
@document.start_element('parent') | ||
@document.start_element('child', [%w[key value]]) | ||
expected = [ | ||
{ child: { key: 'value' } }, | ||
{ child: { key: 'value' } }, | ||
{ key: 'value' } | ||
] | ||
|
||
_(@document.stack).must_equal(expected) | ||
end | ||
|
||
it 'handles multiple children elements correctly' do | ||
@document.start_element('parent') | ||
@document.start_element('child1') | ||
@document.end_element('child1') | ||
@document.start_element('child2', [%w[key value]]) | ||
@document.end_element('child2') | ||
expected = { | ||
child1: '', | ||
child2: { key: 'value' } | ||
} | ||
|
||
_(@document.stack.first).must_equal(expected) | ||
end | ||
|
||
it 'handles text content within elements' do | ||
@document.start_element('parent') | ||
@document.characters('some text') | ||
@document.end_element('parent') | ||
|
||
expected = { parent: 'some text' } | ||
_(@document.stack.first).must_equal(expected) | ||
end | ||
end | ||
end |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to drop this and use the shared one again I think once we are satisfied (though perhaps we'll want to update the shared one with RUBYOPTS in some way also).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geemus I just had the idea, but I can create a shared config on
fog/.github
forfog-aws
fog-xml
andfog-core
.and once all gems have migrated to this new shared config. we can completely remove the old config.
If you agree with this, please let me know and I'll go ahead and create a PR on
fog/.github
👌There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to figure out some specifics, but yeah that sounds fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also or instead, we could leave this for now, but also add the shared workflow back (so that it just runs both). I didn't want to lose the shared workflow. Both is probably redundant, but it would be ok as an interim step.