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

Allowing passing a block to Warning.clear #20

Merged
merged 1 commit into from
Jul 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions lib/warning.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,41 @@ module Processor

# Clear all current ignored warnings, warning processors, and duplicate check cache.
# Also disables deduplicating warnings if that is currently enabled.
def clear
synchronize do
@ignore.clear
@process.clear
@dedup = false
#
# If a block is passed, the values are only cleared within the block and
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This docs is kind of clumsy and needs to be improved 😅

# restored after the block ends.
#
# Examples:
#
# # Clear all values
# Warning.clear
#
# # Clear values only within the block.
# Warning.clear do
# ...
# end
# # The values are stored to previous state.
def clear(&block)
if block
begin
ignore = @ignore.dup
process = @process.dup
dedup = @dedup.dup
clear
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love the recursive call but it's convenient. Alternatively, we could move the clear logic into a private method clear_state or similar.

Copy link
Owner

Choose a reason for hiding this comment

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

This recursive call seems fine, considering it only recurses a single level.

yield
ensure
synchronize do
@ignore = ignore
@process = process
@dedup = dedup
end
end
else
synchronize do
@ignore.clear
@process.clear
@dedup = false
end
end
end

Expand Down
86 changes: 86 additions & 0 deletions test/test_warning.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,92 @@ def teardown
Warning.clear
end

def test_warning_clear_ignore
Warning.ignore(/.*/)

assert_warning '' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To reduce horizontal space we could use:

Suggested change
assert_warning '' do
assert_warning('') { Warning.warn 'foo' }

Not sure if this is compatible with the style of this gem though 🤷

Copy link
Owner

Choose a reason for hiding this comment

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

I think this is fine how it is.

Warning.warn 'foo'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to use Warning.warn because otherwise tests for Ruby 2.4 https://github.com/jeremyevans/ruby-warning/runs/7250088099?check_suite_focus=true would fail.

It's also worth noting that warn "foo" returns a newline while Warning.warn "foo" does not so no need to assert "foo\n".

I guess this is not related to this warning gem but the Ruby's Warning module:

$ irb
>> warn "foo"
foo
=> nil
>> Warning.warn "foo"
foo=> nil

Copy link
Owner

Choose a reason for hiding this comment

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

It's expected that Kernel#warn operates differently from Warning.warn, so this isn't an issue.

end

Warning.clear do
assert_warning 'foo' do
Warning.warn 'foo'
end
end

assert_warning '' do
Warning.warn 'foo'
end

Warning.clear

assert_warning 'foo' do
Warning.warn 'foo'
end
end

def test_warning_clear_process
Warning.process('', /foo/ => :raise)

e = assert_raises(RuntimeError) do
Warning.warn 'foo'
end
assert_equal('foo', e.message)

Warning.clear do
assert_warning 'foo' do
Warning.warn 'foo'
end
end

e = assert_raises(RuntimeError) do
Warning.warn 'foo'
end
assert_equal('foo', e.message)

Warning.clear

assert_warning 'foo' do
Warning.warn 'foo'
end
end

def test_warning_clear_dedup
Warning.dedup

assert_warning 'foo' do
Warning.warn 'foo'
end

assert_warning '' do
Warning.warn 'foo'
end

Warning.clear do
assert_warning 'foo' do
Warning.warn 'foo'
end

assert_warning 'foo' do
Warning.warn 'foo'
end
end

assert_warning '' do
Warning.warn 'foo'
end

Warning.clear

assert_warning 'foo' do
Warning.warn 'foo'
end

assert_warning 'foo' do
Warning.warn 'foo'
end
end

def test_warning_dedup
gvar = ->{$test_warning_dedup}

Expand Down