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

Include GetOAuth* methods in Client's method set #803

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vibridi
Copy link

@vibridi vibridi commented Sep 22, 2020

Fixes #744 .

This PR moves the previously static GetOAuth* methods into Client's method set. This permits to use the configurable API endpoint instead of the global constant, and the existing HTTP client that is set on the Client.

This PR currently doesn't include unit tests, as no such tests existed before either. Please advise if you want me to add tests.

Note: this PR breaks backwards compatibility.

Verification please.

@kanata2
Copy link
Member

kanata2 commented Sep 29, 2020

Thank you for your PR.

I agree that the testability is low due to the dependency on APIURL constant value, but this function itself does not seem to be a feature that slack.Client should have 🤔
So, it would be nice to have some other ideas to solve this problem...

@kanata2 kanata2 added feedback given When a review has been conducted and awaiting the response from the comitter(s) thinking labels Sep 29, 2020
@vibridi
Copy link
Author

vibridi commented Sep 30, 2020

@kanata2
thanks for your feedback

does not seem to be a feature that slack.Client should have

why not?
Even if this is the OAuth flow, it still belongs to the Slack API with the same base path, as usage of APIURL constant shows.

An alternative approach is to declare a different client, let's say slack.OAuthClient, and add these funcs to its method set, but that would require duplicating a bunch of code...

As a matter of fact, I'm not sure why you designed these OAuth funcs as static.

@kanata2 kanata2 modified the milestone: v0.7.0 Oct 2, 2020
@kanata2 kanata2 removed the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow mocking requests to GetOAuth* methods
2 participants