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

WatchersCount on repository incorrect #699

Closed
DaveWM opened this issue Feb 11, 2015 · 12 comments
Closed

WatchersCount on repository incorrect #699

DaveWM opened this issue Feb 11, 2015 · 12 comments

Comments

@DaveWM
Copy link
Contributor

DaveWM commented Feb 11, 2015

It looks like the WatchersCount property is set incorrectly on the repository object, it's always the same as the StargazersCount property. For example, it is showing as 605 for this repo when it should be 80. Will submit a PR for this when I have time :).

@shiftkey
Copy link
Member

@DaveWM I think this is actually a bug in the GitHub API - https://api.github.com/repos/octokit/octokit.net

cc @pengwynn

@pengwynn
Copy link

It's confusing but that's expected, because history.

You'll find what the UI calls "Watchers" in the subscribers_count field for API resources.

@shiftkey
Copy link
Member

@pengwynn thanks for the details

@DaveWM
Copy link
Contributor Author

DaveWM commented Feb 11, 2015

@shiftkey @pengwynn Thanks guys :)

@haacked
Copy link
Contributor

haacked commented Feb 11, 2015

@shiftkey @DaveWM if I understand this correctly, it seems like we should remove the deprecated property from our client API. I'm guessing WatchersCount can go away as long as we have a SubscribersCount property?

@haacked haacked reopened this Feb 11, 2015
@shiftkey
Copy link
Member

@haacked gah, you're right - we can still fix this.

We could map subscribers_count to WatchersCount instead of deprecating the property altogether. How does that feel?

@haacked
Copy link
Contributor

haacked commented Feb 11, 2015

I'd rather have the property name match the api field name as much as possible. We're not 1.0 yet, we can do whatever the hell we want. 😛 Let's just match the API canonical names.

@shiftkey
Copy link
Member

Fiiiiine 😛

@haacked
Copy link
Contributor

haacked commented Feb 11, 2015

😛 It's just my preference. I think it's a good rule of thumb. But I'm open to strong arguments to the contrary in specific cases. 😄

@haacked
Copy link
Contributor

haacked commented Feb 11, 2015

Also, if we're all in agreement, would you like to submit a PR with this change @DaveWM?

@shiftkey
Copy link
Member

It's just my preference. I think it's a good rule of thumb.

I'm just being pre-caffeine silly here. As the behaviour isn't going to change for the lifetime of v3, I'm 👍 with hiding away this incorrect field.

@DaveWM
Copy link
Contributor Author

DaveWM commented Feb 11, 2015

@haacked @shiftkey I've made a PR (#701), just removed the WatchersCount property. Probably a good idea to remove this even though it's still on the api, just to avoid confusion

shiftkey added a commit that referenced this issue Feb 23, 2015
removed WatchedCount property from Repository, resolves #699
ryangribble added a commit to TattsGroup/octokit.net that referenced this issue Sep 25, 2016
…starred count and not the watching count (which is in SubscribersCount). See octokit#699 for more info
ryangribble added a commit that referenced this issue Jan 15, 2017
…Size (#1473)

* Add some missing fields to Repository object

* Remove WatchersCount as due to historic reasons this is actually the starred count and not the watching count (which is in SubscribersCount).  See #699 for more info
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

No branches or pull requests

4 participants