Skip to content

Conversation

@aaronbloomfield
Copy link

As per https://developer.github.com/v3/activity/watching/, it seems the "watched" part of the URL to handle watching and unwatching a repo should be "subscriptions" instead. The updated URL (in this pull request) worked fine for me; the original one didn't.

@Nek-
Copy link
Contributor

Nek- commented Sep 3, 2014

Thank you :) .

But method names should be change as well but old methods proxies to new ones.

Also please fixed test and documentation into your PR and it will be perfect :) .

@stof
Copy link
Contributor

stof commented Sep 17, 2014

@Nek- the github UI still talks abotu watching, so it may be fine to keep watch() and unwatch()

@stof
Copy link
Contributor

stof commented Sep 17, 2014

@aaronbloomfield please update the tests as well

@Nek-
Copy link
Contributor

Nek- commented Sep 18, 2014

@stof disagree with you. Github keep them only to keep BC. Our methods will fail as soon as they think it's time to remove theses entries. To be sure to provide data as expected, IMO we should set a proxy to the new method.

Why do you think that it's better to keep old entry point ?

@stof
Copy link
Contributor

stof commented Sep 18, 2014

I'm not talking about keeping the implementation based on the old endpoint, but about keeping the name watch and unwatch, as it is what the UI uses:
github_unwatch

@aaronbloomfield
Copy link
Author

(sorry for the delay on this)

I've added a subscribe() and unsubscribe() method, while keeping the watch() and unwatch() method; the latter two have been marked in the comments as deprecated. This should allow existing code to continue to use watch() and unwatch().

The tests were all updated (and ones added for subscribe() and unsubscribe()), and TravisCI is happy!

@Nek-
Copy link
Contributor

Nek- commented Sep 18, 2014

👍 Thanks

Note for merger: the documentation should be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to write it as @deprecated use unsubscribe() instead, to match common deprecation messages

@cursedcoder
Copy link
Contributor

fixed af276bc

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.

4 participants