allow HTTP headers to be supplied for outcome request#73
allow HTTP headers to be supplied for outcome request#73hmoffatt wants to merge 5 commits intopylti:masterfrom
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
ryanhiebert
left a comment
ryanhiebert
left a comment
There was a problem hiding this comment.
This looks like a generally good change, thank you for bringing it up! There are a couple things that need to be addressed before we can merge this.
First, using headers={} as defaults in function signatures is a bad idea, because that dict instance is preserved and mutable. See https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments for more on this.
Second, it needs some tests. I don't use this feature in my own work right now, so its that much more important that there are good automated tests. This seems like it may be a less-tested part of the code, but I'll be satisfied when the lines added here are covered.
Finally, should the headers be passed in for each individual method, or would it be best to have them in the __init__ and from_post_request constructors? Since your use-case is about overriding the user agent in particular, it seems like the class-level might be the better place for it.
|
Thanks for the feedback! I have addressed all three issues in my latest changes. |
|
Did you have a chance to review this @ryanhiebert ? |
ryanhiebert
left a comment
ryanhiebert
left a comment
There was a problem hiding this comment.
Thanks for adding some tests there. I'm not sure what's going on with the tests, but I'm going to need to get that figured out before I'm ready to merge.
src/lti/outcome_request.py
Outdated
| "Invalid outcome request option: {}".format(key) | ||
| ) | ||
|
|
||
| self.headers = CaseInsensitiveDict(headers) |
There was a problem hiding this comment.
Since headers may be None, will this actually work? It's not typical for that to work for dict. I'd probably prefer this to be written:
| self.headers = CaseInsensitiveDict(headers) | |
| self.headers = CaseInsensitiveDict(headers or {}) |
There was a problem hiding this comment.
requests' CaseInsensitiveDict handles that case, but I've added the code you suggested for clarity.
There was a problem hiding this comment.
I've extended the test a bit further so there's a net improvement in the test coverage.
|
Hmm. It seems like CI isn't doing anything. I'll need to see what I can do about that. |
|
Sorry, accidentally pushed an unrelated change. I have removed it and also squashed my previous commits down to one. |
2 participants
I need to be able to override the User-Agent in the POST request because an outcome service I'm reporting to is rejecting the default python-requests user agent.