Set HTTP GET body to nil, resolves #56 #57
Conversation
| # The run_request() method body argument defaults to {}, which is incorrect for GET requests | ||
| # https://github.com/technicalpickles/pager_duty-connection/issues/56 | ||
| # NOTE: PagerDuty support discourages GET requests with bodies, but not throwing an ArgumentError to prevent breaking | ||
| # corner-case implementations. |
There was a problem hiding this comment.
I'd love to do a logger.warn here as I think a raise(ArgumentError) would be too aggressive and potentially breaking, but I don't think we have a logger. I see some logging in the Faraday setup but I don't think that's easily usable. Let me know if there is a logger or if we're okay throwing an ArgumentError if someone tries to do a HTTP GET request with a body.
There was a problem hiding this comment.
We have activesupport as a dependency, so we could use it's deprecation support for it.
There was a problem hiding this comment.
Neat. I think I might have to leave that to someone else as I'm more of a pure Rubyist, so I have little experience with the Rails Active Library ecosystem. While that Deprecation system looks simple on the surface I'm not clear on exactly which deprecation method is needed to deprecate an argument to a method. I see ActiveSupport::Deprecation also needs a gem version, which I assume would be 3.0? Might be best if I leave this to someone who knows their way around better.
| # The run_request() method body argument defaults to {}, which is incorrect for GET requests | ||
| # https://github.com/technicalpickles/pager_duty-connection/issues/56 | ||
| # NOTE: PagerDuty support discourages GET requests with bodies, but not throwing an ArgumentError to prevent breaking | ||
| # corner-case implementations. |
There was a problem hiding this comment.
We have activesupport as a dependency, so we could use it's deprecation support for it.
| unless body.nil? | ||
| TIME_KEYS.each do |key| | ||
| if body.has_key?(key) | ||
| body[key] = body[key].iso8601 if body[key].respond_to?(:iso8601) | ||
| end |
There was a problem hiding this comment.
This middleware is very narrowly scoped, so think it is just as easy, and more readable, to have a guard clause to return early:
| unless body.nil? | |
| TIME_KEYS.each do |key| | |
| if body.has_key?(key) | |
| body[key] = body[key].iso8601 if body[key].respond_to?(:iso8601) | |
| end | |
| return unless body | |
| TIME_KEYS.each do |key| | |
| if body.has_key?(key) | |
| body[key] = body[key].iso8601 if body[key].respond_to?(:iso8601) | |
| end |
There was a problem hiding this comment.
This will skip the @app.call on line 78. That's why I didn't use a guard clause, as I didn't want to duplicate that line of code. Is that @app.call env line safe to skip in this case? I'm not familiar with Faraday Middleware.
There was a problem hiding this comment.
Ah, right you are. I missed that at the bottom of the method. I still like guard clauses, but that can be a followup. Going to merge to be able to release this.
| unless body.nil? | ||
| TIME_KEYS.each do |key| | ||
| if body.has_key?(key) | ||
| body[key] = body[key].iso8601 if body[key].respond_to?(:iso8601) | ||
| end |
There was a problem hiding this comment.
Ah, right you are. I missed that at the bottom of the method. I still like guard clauses, but that can be a followup. Going to merge to be able to release this.
Tested in our client app which reads the
schedulesandusersendpoints.