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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented oauth-header wrap #7

Merged
merged 2 commits into from
Jun 1, 2022
Merged

Conversation

deas
Copy link

@deas deas commented Apr 15, 2020

clj-http supports it and it is quite helpful to be compatible. 馃槈

@martinklepsch
Copy link
Collaborator

Hi & thanks for contributing! 馃帀

Is the implementation of this middleware identical to the implementation in clj-http? A link to the original implementation would be helpful to review & consider this. 馃槈

@deas
Copy link
Author

deas commented Apr 16, 2020

TBH, I was only looking at clj-http to make sure we get similar behavior.

However, most of the code is copied & pasted directly from clj-http-lite because it looked more uniform to me. 馃槈

@martinklepsch
Copy link
Collaborator

Yeah I think I understood that, just reviewing this kind of thing it helps when you point me towards where this code comes from etc. I was looking for this link: https://github.com/dakrone/clj-http/blob/5154fcc6b24a9ce4bbff6ba45c971c19976491a3/src/clj_http/client.clj#L835-L850

I'm maintaining this library in my free time so anything that helps reviewing PRs is greatly appreciated.

Would you mind updating the changelog with a line detailing that :oauth-token will be set to the header? Technically this is a breaking change (users might be using :oauth-token with their own middleware) so let's document it properly. Please also link to this PR in the changelog. You can use "unreleased" as the header in the changelog.

Thank you

@deas
Copy link
Author

deas commented Apr 16, 2020

NP, Hope we are good to go now. 馃槈

CHANGELOG.md Show resolved Hide resolved
test/clj_http/test/client.clj Outdated Show resolved Hide resolved
@borkdude
Copy link
Collaborator

@deas Can you make this PR up to date with master?

@borkdude borkdude merged commit 1228fc8 into clj-commons:master Jun 1, 2022
@borkdude
Copy link
Collaborator

borkdude commented Jun 1, 2022

@deas Sorry it took me so long to merge this, I didn't get a notification about it.

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.

None yet

3 participants