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

Conversation

splattael
Copy link
Contributor

If a block is passed, the values are only cleared within the block and
restored after the block ends.

Fixes #18.

@splattael splattael changed the title Allowing passing a block to clear Allowing passing a block to Warning.clear Jul 8, 2022
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.

@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 😅

Warning.ignore(/.*/)

assert_warning '' do
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.

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.

If a block is passed, the values are only cleared within the block and
restored after the block ends.
Copy link
Owner

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for working on it.

def test_warning_clear_ignore
Warning.ignore(/.*/)

assert_warning '' do
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.ignore(/.*/)

assert_warning '' do
Warning.warn 'foo'
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.

ignore = @ignore.dup
process = @process.dup
dedup = @dedup.dup
clear
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.

@jeremyevans jeremyevans merged commit f8b2f1c into jeremyevans:master Jul 8, 2022
kachick added a commit to kachick/ruby-ulid that referenced this pull request Jul 15, 2022
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.

Allow warnings to be clear only within a block
2 participants