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

Specify User-Agent on every HTTP request (#3628) #3682

Merged
merged 1 commit into from
Dec 23, 2017

Conversation

igrep
Copy link
Contributor

@igrep igrep commented Dec 20, 2017

Fix: #3628

Wrap up functions in Network.HTTP.Client and Network.HTTP.Simple to
add User-Agent to the request object.

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

Testing the change

I confirmed some requests are fixed by stack setup, stack install, stack new.
But I haven't tested all HTTP requests made by stack.

For reviewers

I'm not sure especially with the name and place of the wrapper module (and the name of its functions).
Feel free to modify for better module structure.

Fix: commercialhaskell#3628

Wrap up functions in `Network.HTTP.Client` and `Network.HTTP.Simple` to
add User-Agent to the request object.
@decentral1se
Copy link
Member

decentral1se commented Dec 21, 2017

This looks like a solid approach to me! Perhaps, to help avoiding using the not wrapped function, we can use a similar HLint trick like in https://github.com/commercialhaskell/stack/blob/master/.hlint.yaml#L44 - where a warning is produced (and the CI will fail) if the wrong function is used - assuming we always want these headers to go out? I assume so! Thanks for the patch!

@igrep
Copy link
Contributor Author

igrep commented Dec 22, 2017

@lwm Thanks for advice!

Perhaps, to help avoiding using the not wrapped function, we can use a similar HLint trick like in https://github.com/commercialhaskell/stack/blob/master/.hlint.yaml#L44

You mean, should I rename functions exported by Network.HTTP.StackClient to make them different from functions in Network.HTTP.Simple and Network.HTTP.Client?
It looks like that hlint can suggest only when lhs and rhs are literally different as their de-qualified names.
I'm new to .hlint.yaml, so tell me if I get something wrong!

If I should rename, I'm going to prefix stack to the new functions: stackHttpJSON, stackHttpLbs, etc.

@decentral1se
Copy link
Member

I'm not exactly sure either ;) I thought you could pass full import paths (like over here) and get away with it. If you can't do that, maybe the renaming with the prefix is a better option in general - less magic!

@mgsloan
Copy link
Contributor

mgsloan commented Dec 23, 2017

Good stuff @igrep , thanks a bunch!

Cool idea bout using hlint to reduce likelihood of regressions, @lwm! Indeed I'm not sure if HLint can distinguish where identifiers are imported from. Seems like it'd be a major weakness if not, I'm guessing it can do this. I've opened #3700 to track implementing the idea.

@mgsloan mgsloan merged commit dd01d0a into commercialhaskell:master Dec 23, 2017
@igrep igrep deleted the user-agent branch December 24, 2017 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify User-Agent to get over my company's firewall
3 participants