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

Make Warning.process(/foo/) { :raise } fail on initial call #17

Closed
splattael opened this issue Jul 5, 2022 · 2 comments · Fixed by #19
Closed

Make Warning.process(/foo/) { :raise } fail on initial call #17

splattael opened this issue Jul 5, 2022 · 2 comments · Fixed by #19

Comments

@splattael
Copy link
Contributor

Problem

I was wondering if Warning.process(/foo/) { :raise } is explicitly supported as it works only if no prior calls to Warning.process has been made.

Consider this code:

require 'warning'

Warning.process(/foo/) { :raise } # This works?
Warning.process(/bar/) { :raise }
# => ArgumentError (comparison of Regexp with Regexp failed)

The first argument of Warning.process must be a path and actions can be passed as a Hash:

require 'warning'

Warning.process('', /foo/ => :raise)
Warning.process('', /bar/ => :raise)
warn("foo") # => RuntimeError (foo)
warn("bar") # => RuntimeError (bar)

Proposed solution

Make Warning.process(/foo/) { :raise } always fail.

Happy to create a PR if the proposed solution is accepted 💪

Refs

Refs https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91789

@jeremyevans
Copy link
Owner

Thanks for the report. Warning.process(regexp) is not supported. It ends up working by accident because when only a single element is in the process array, sort_by! does not raise. We cannot support both regexps and strings as the path argument, because the algorithm used needs the paths sorted.

I'm generally against defensive programming type checks in Ruby, but I'm OK with a type check in this case. Can you work on a PR for this?

@splattael
Copy link
Contributor Author

@jeremyevans Yay, thanks for the quick response 🙇

See #19 🎉

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 a pull request may close this issue.

2 participants