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

Set correct uplevel to warning on supply_default_content_type #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pocke
Copy link
Member

@pocke pocke commented Apr 1, 2022

This PR fixes uplevel of supply_default_content_type.

Problem

Currently supply_default_content_type specifies an incorrect uplevel to warn method.
For example:

require 'net/http'

url = URI.parse('http://www.example.com/index.html')
req = Net::HTTP::Post.new(url.path)
res = Net::HTTP.start(url.host, url.port) {|http|
  http.request(req)
}
res.body
$ ruby test.rb
/Users/kuwabara.masataka/.rbenv/versions/trunk/lib/ruby/3.2.0+0/net/http/generic_request.rb:186: warning: net/http: Content-Type did not set; using application/x-www-form-urlencoded

The warning points to net/http/generic_request.rb which is not written by the user. The location is not helpful to fix the warning by the user.

Solution

Find the correct uplevel from the caller and specify it.

I also considered using a fixed uplevel for it, like uplevel: 6. But I think caller_location is better than the fixed value.
Because the uplevel is too deep, and supply_default_content_type is called from multiple places so the fixed value doesn't work properly for all callers perhaps.

@mame
Copy link
Member

mame commented Jul 28, 2022

@ko1 said that absolute_path might return nil if a method written in C is called

@@ -298,7 +298,10 @@ def flush_buffer(out, buf, chunked_p)

def supply_default_content_type
return if content_type()
warn 'net/http: Content-Type did not set; using application/x-www-form-urlencoded', uplevel: 1 if $VERBOSE
if $VERBOSE
uplevel = caller_locations.find_index { |loc| not loc.absolute_path.match?(%r!net/http!) } || 0
Copy link
Member

Choose a reason for hiding this comment

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

Maybe:

Suggested change
uplevel = caller_locations.find_index { |loc| not loc.absolute_path.match?(%r!net/http!) } || 0
uplevel = caller_locations.find_index { |loc| not loc.absolute_path&.match?(%r!net/http!) } || 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants