-
-
Notifications
You must be signed in to change notification settings - Fork 753
Include the spec location in warnings if we can't generate a call site. #1253
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,38 @@ | ||
require "rspec/support/warnings" | ||
|
||
module RSpec | ||
module Core | ||
module Warnings | ||
# @private | ||
# | ||
# Used internally to print deprecation warnings | ||
def deprecate(deprecated, data = {}) | ||
RSpec.configuration.reporter.deprecation( | ||
{ | ||
:deprecated => deprecated, | ||
:call_site => CallerFilter.first_non_rspec_line | ||
}.merge(data) | ||
) | ||
end | ||
|
||
# @private | ||
# | ||
# Used internally to print deprecation warnings | ||
def self.deprecate(deprecated, data = {}) | ||
RSpec.configuration.reporter.deprecation( | ||
{ | ||
:deprecated => deprecated, | ||
:call_site => CallerFilter.first_non_rspec_line | ||
}.merge(data) | ||
) | ||
end | ||
# @private | ||
# | ||
# Used internally to print deprecation warnings | ||
def warn_deprecation(message) | ||
RSpec.configuration.reporter.deprecation :message => message | ||
end | ||
|
||
# @private | ||
# | ||
# Used internally to print deprecation warnings | ||
def self.warn_deprecation(message) | ||
RSpec.configuration.reporter.deprecation :message => message | ||
end | ||
def warn_with(message, options = {}) | ||
if options[:use_spec_location_as_call_site] | ||
message += "." unless message.end_with?(".") | ||
|
||
if RSpec.current_example | ||
message += " Warning generated from spec at `#{RSpec.current_example.location}`." | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These message additions depend on the message ending with a period to read well. Are we sure that's the case everywhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth making this append a period to the message if it lacks one before adding the extra bit so it's not a concern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dealt with, but GitHub hasn't squashed this comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's generally a bad idea to mutate a passed arg but your implementation here will mutate Also, I'm not sure the "RSpec could not determine which call generated this warning" bit has any value -- to a user, being told "I can't tell you any more about this" isn't useful or helpful. There's nothing actionable for the user based on whether or not rspec can add a call site. It would be a bit like if grep added a "can't find any more matches" message after everytime you ran it. If there's no example, I don't think it should append anything. |
||
end | ||
|
||
super(message, options) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation can still mutate the passed
I think you can just change |
||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are cases where there's not a current example (and thus, it will return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah I see it... |
||
end | ||
end |
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.
In your rspec-support PR it has already extended this module on to
RSpec
-- so why extend it ontoRSpec
a second time here?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.
This is RSpec::Core::Warnings, which is not extended from RSpec support. As such it's needed here. The tests fail without this line.
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.
Sorry, I misread this as
extend RSpec::Support::Warnings
. Ignore me.