Initial support for customHeaders #103
Conversation
There was a problem hiding this comment.
@dayures, I have handed over the maintenance of this repository to @wikier a while ago (I have moved to different areas), so my comment may not carry lots of weight. However, having looked at the changes, they seem pretty fine to me and if you do have use cases where such additional custom headers are necessary for a SPARQL endpoint, then I agree these are also necessary.
| @@ -349,6 +351,18 @@ def setOnlyConneg(self, onlyConneg): | |||
| """ | |||
| self.onlyConneg = onlyConneg | |||
|
|
|||
| def setCustomHeaders(self, customHeaders): | |||
There was a problem hiding this comment.
why not also a addCustomHeader(self, header, value) method?
There was a problem hiding this comment.
I like the idea. I am going to change the code in order to take this approach!
| @@ -704,6 +718,10 @@ def _createRequest(self): | |||
| raise NotImplementedError("Expecting one of: {0}, but received: {1}".format(valid_types, | |||
| self.http_auth)) | |||
|
|
|||
| # The header field name is capitalized in the request.add_header method. | |||
There was a problem hiding this comment.
Do we want to give preference to our internal headers or to the custom ones? Because then the order to request.add_header() matters.
There was a problem hiding this comment.
In this concrete development, the custom ones have more preference than the internal ones (in case somebody want to highly customize the request).
That's why it is warned in the method documentation (line 355)
|
I've added few minor comments to the PR. But otherwise it LGTM. |
|
Finally, the code was changed slightly (see 66fbbbc), so this PR is not merged. |
|
thx |
@wikier @indeyets @iherman , do you agree this approach? (all HTTP headers could be overriden)