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

avoid duplicate "header" kwarg #144

Closed

Conversation

cedricvanrompay
Copy link

Fixes #133

If headers were passed during call to requests_document_loader,
these headers will be forced in call to requests.get.

Fixes digitalbazaar#133

If headers were passed during call to `requests_document_loader`,
these headers will be forced in call to `requests.get`.
headers = {
'Accept': 'application/ld+json, application/json'
}
response = requests.get(url, headers=headers, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

It is unclear how this PR is changing the behavior. From what I am seeing, even before the PR a user could do

requests_document_loader(…, headers={
    'Foo': 'bar',
})

it should be working on implementation at master. Or am I misunderstanding it?


headers = {} if 'headers' in kwargs else ({
'headers': (
options.get('headers', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we're addressing both kwargs['headers'] and options['headers'], where should those be found?

@@ -14,6 +14,9 @@

from pyld.jsonld import (JsonLdError, parse_link_header, LINK_HEADER_REL)

DEFAULT_HEADERS = {
'Accept': 'application/ld+json, application/json'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I appreciate factoring this out as a global constant.

@BigBlueHat
Copy link
Contributor

Pretty sure this just avoids the code path where the HTML media types are added. Also, #170 has some fixes related to #133 and I believe that's now been fixed upstream by @pchampin and/or @iherman

So, gonna close this one in favor of those.

@BigBlueHat BigBlueHat closed this Feb 5, 2024
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.

Could not retrieve a JSON-LD document from the URL.
3 participants