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

Added some AccessToken typehints where appropriate #360

Closed

Conversation

rtheunissen
Copy link
Contributor

Unless there is a good reason not to do this?

@ramsey
Copy link
Contributor

ramsey commented Jun 29, 2015

I think in some cases, we were expecting that the provider might pass the string access token instead of the object to some of these methods. If we decide they must always pass an AccessToken, then I'm fine with that.

@shadowhand, any thoughts?

@rtheunissen
Copy link
Contributor Author

I believe it's in our interest to be strict with parameter types and provide flexibility with clever methods instead. So keep the rules and policies specific, but give providers enough access to adjust things accordingly.

However, if there is a use case or requirement for using a string, then I'm happy to document the expected type as @param AccessToken|string.

@ramsey
Copy link
Contributor

ramsey commented Jun 29, 2015

I agree with you. I'm just trying to make sure I'm not forgetting something. :-)

@rtheunissen
Copy link
Contributor Author

Absolutely, there's no urgency for this so I don't mind hanging around for a while.

* @return array
*/
protected function getDefaultHeaders($token = null)
protected function getDefaultHeaders(AccessToken $token = null)
Copy link
Member

Choose a reason for hiding this comment

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

As per my comments on #359, this method shouldn't even have a $token parameter.

@shadowhand
Copy link
Member

👎 for the reasons I listed. This offers no real benefits for end users.

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