Remove User-Agent if empty or Nil #228

Merged
merged 1 commit into from Apr 15, 2013

Projects

None yet

2 participants

@mika0x56

Hi.

I think that about covers it.
I also made an entry to ChangeLog. Hope that was appropriate!

Cheers,
Mika

@igrigorik
Owner

While I'm technically not opposed to this.. what's the motivating use case? As a rule of thumb, I always encourage everyone to specify a UA header to identify their scripts and crawlers (helps tracks down numerous bugs and abuse cases).

@mika0x56

Starting at one end of the issue, I see a tool for what it is and I prefer it to offer as much as possible within its domain. As it doesn't make any sense to supply an empty UA header, and I can't really motivate forbidding it (to some part, any other lib I can think of has this functionality), It should be offered as a possibility. At the other end we have the issue of abuse. Every tool can be abused. Will forbidding no UA prevent this abuse? Most likely they'll just find another lib to do it. And for sure, some of the greatest inventions can be abused, but that usually doesn't justify stopping (Wikipedia comes to mind).

In addition, there are some reasonable use cases. Downloading e-legimitation software from Swedish Telia cannot be done unless the client uses the "right" combination of OS (Ubuntu 12.04) and Firefox. I could argue that they are abusing my UA header as their software runs just as fine on 12.10. Another example could be mocking clients for unit tests.

To sum up. Let's just try to build good stuff :-)

@igrigorik igrigorik commented on an outdated diff Apr 13, 2013
lib/em-http/client.rb
@@ -154,7 +154,11 @@ def build_request
head['host'] ||= encode_host
# Set the User-Agent if it hasn't been specified
- head['user-agent'] ||= "EventMachine HttpClient"
+ if !head.key?('user-agent')
@igrigorik
igrigorik Apr 13, 2013 owner

Based on discussion in #206, how about:

  • if UA is not specified, use default
  • if empty string is specified - keep it (as discussed in bug above)
  • if nil, then omit header

(side note: can you switch to .nil?)

@mika0x56

Hi. I've finished the requested changes and squashed them with previous commit to keep a clean history. Let me know what you think. Cheers!

@igrigorik igrigorik merged commit 8fee832 into igrigorik:master Apr 15, 2013
@igrigorik
Owner

Awesome, thanks @mikaelwikman!

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