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

feat: add headers when loading URLs, closes #816 #826

Merged
merged 15 commits into from
Dec 31, 2022
Merged

Conversation

amrbashir
Copy link
Member

@amrbashir amrbashir commented Dec 27, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@amrbashir amrbashir requested a review from a team as a code owner December 27, 2022 20:40
lucasfernog
lucasfernog previously approved these changes Dec 31, 2022
lucasfernog
lucasfernog previously approved these changes Dec 31, 2022
@lucasfernog
Copy link
Member

Initially I thought with_headers would apply to all URLs which is not the case, so maybe we chould change the signature to with_url_and_headers.

@lucasfernog
Copy link
Member

I've changed the signatures to use the existing methods instead of adding new ones. So now with_url, with_html and load_url takes an Option<http::HeaderMap>. Let me know what you think @amrbashir I think this is less confusing to users.

@lucasfernog lucasfernog changed the title feat: add load_url_with_headers, closes #816 feat: add headers when loading URLs, closes #816 Dec 31, 2022
@amrbashir
Copy link
Member Author

There is no point in using headers with with_html since it loads local content.
Also, most users won't use headers, so I think it should be a separate function. I like the idea of with_url_and_headers instead.

@lucasfernog
Copy link
Member

Cool @amrbashir

@amrbashir amrbashir merged commit 8ae93b9 into dev Dec 31, 2022
@amrbashir amrbashir deleted the feat/load-with-headers branch December 31, 2022 22:45
@github-actions github-actions bot mentioned this pull request Dec 31, 2022
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.

2 participants