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

[Http] add default User-Agent string to cURL requests #96

Merged

Conversation

seanreinhardtapps
Copy link
Contributor

Copy link
Member

@amorde amorde left a comment

Choose a reason for hiding this comment

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

thanks for working on this!

"cocoapods/#{version}"
end

# @return [String] Shell command to add the custom User-Agent.
Copy link
Member

Choose a reason for hiding this comment

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

why does this say it's a shell command? I thought -A accepts a string to specify as the user agent

this also seems specific to Http because it's only used for cURL, so maybe it shouldn't be in Base at all


# @return [String] cURL command flag to add the custom User-Agent.
#
def self.user_agent_command
Copy link
Member

Choose a reason for hiding this comment

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

maybe user_agent_argument?

# @return [String] the User-Agent string
#
def self.user_agent_string version = Pod::VERSION
"cocoapods/#{version}"
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we make this CocoaPods/#{version}?

private

executable :curl

def download_file(full_filename)
parameters = ['-f', '-L', '-o', full_filename, url, '--create-dirs', '--netrc-optional', '--retry', '2']

parameters << user_agent_argument if headers.nil? || headers.none? { |header| header.include?(USER_AGENT_HEADER) }
Copy link
Member

Choose a reason for hiding this comment

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

I think header names are case-insensitive, so should we do a case-insensitive comparison here?

#
# @return [String] the User-Agent string
#
def self.user_agent_string(version = Pod::VERSION)
Copy link
Member

Choose a reason for hiding this comment

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

I dont think Pod::VERSION is actually defined in this gem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not. I've been trying to get a feel for the independence of this Gem from the CocoaPods gem, and have updated to a safer access of Pod::VERSION to try to provide the expected general-case version without the access concern.

Expanding a bit beyond the feature request, I added the the cocoapods-downloader gem version to the User-Agent, so that it looks like: CocoaPods/1.9.1 cocoapods-downloader/1.3.0. If this is not preferred, I'm happy to revert.

Copy link
Member

Choose a reason for hiding this comment

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

Nice! seems worthwhile to include that

CHANGELOG.md Outdated
@@ -4,7 +4,9 @@

##### Enhancements

* None.
* Add User-Agent to `cURL` requests when downloading source via the `:http` download strategy, unless one was provided by the `:headers` option.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, please add two empty spaces at the end of this line so markdown formatting works.

@seanreinhardtapps seanreinhardtapps force-pushed the sr-add-default-user-agent branch from 029d9fc to 8179b8f Compare June 7, 2020 19:33
@amorde amorde merged commit 519dabd into CocoaPods:master Jun 7, 2020
@amorde
Copy link
Member

amorde commented Jun 7, 2020

Thanks @seanreinhardtapps!

@seanreinhardtapps seanreinhardtapps deleted the sr-add-default-user-agent branch June 30, 2020 19:34
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 this pull request may close these issues.

feature request: report a more usefull user-agent when doing http requests
4 participants