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

make GetAllContributors return a custom dto #614

Merged
merged 1 commit into from
Dec 10, 2014
Merged

make GetAllContributors return a custom dto #614

merged 1 commit into from
Dec 10, 2014

Conversation

SimonCropp
Copy link
Contributor

fixes #610

@SimonCropp
Copy link
Contributor Author

@shiftkey @haacked so there was already a Contributor so i created a RepositoryContributor. not happy with it, any better ideas?

@@ -22,7 +22,7 @@ internal string DebuggerDisplay
get
{
return String.Format(CultureInfo.InvariantCulture,
"Author: Id: {0} Login: {1}", Author.Id, Author.Login);
"Contributor Statistics: Id: {0} Login: {1}", Author.Id, Author.Login);
Copy link
Member

Choose a reason for hiding this comment

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

Is the Statistics step here a refactoring typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@shiftkey
Copy link
Member

shiftkey commented Dec 7, 2014

Re: naming things, I can see the documentation refers to Collaborators rather than Contributors in the API:

https://developer.github.com/v3/repos/collaborators/

But on the website they're called Contributors

https://github.com/octokit/octokit.net/graphs/contributors

In An Ideal World, I'd like the names of our things to reflect the API directly, but I'm happy to take RepositoryContributor at the moment until we get a chance to revisit this all the places we use identity - User/Author/Assignee/Members/Collaborator/Contributor all come to mind.

@@ -46,7 +46,7 @@ public class Author

public bool SiteAdmin { get; set; }

internal string DebuggerDisplay
internal virtual string DebuggerDisplay
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason to virtual-ize these? Should we be doing it for all of them?

@shiftkey
Copy link
Member

shiftkey commented Dec 7, 2014

A couple of little polish things - the rest look good ✨

@SimonCropp
Copy link
Contributor Author

@shiftkey ok i think i have addressed all your concerns. I also squashed

@SimonCropp
Copy link
Contributor Author

Any timeline on the release this might be included in. Just wondering if I can delay my usage so I can just run with the proper nuget

@shiftkey
Copy link
Member

shiftkey commented Dec 8, 2014

I'd like to get 0.5.3 release out (which is just #605) but there's been extra things land in master recently which I'd like to tighten up before shipping a 0.6.x release.

  • Optimist Me- next week sometime
  • Pessimist Me - add a week to the optimistic guess

shiftkey added a commit that referenced this pull request Dec 10, 2014
make GetAllContributors return a custom dto
@shiftkey shiftkey merged commit 7c04fe5 into octokit:master Dec 10, 2014
@mkchandler
Copy link

I see this was merged in to master and v0.5.3 was released, but I'm not seeing these changes in the v0.5.3 NuGet package. Was it somehow withheld from the release?

@shiftkey
Copy link
Member

@mkchandler my apologies, it didn't make it into 0.5.3 - https://github.com/octokit/octokit.net/releases/tag/v0.5.3

It'll be out in 0.6.0 along with some other fixes - #621

@mkchandler
Copy link

Sounds good 👍 Thanks

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.

GetAllContributors needs custom DTO
3 participants