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

IUserKeysClient.GetAll -> GetAllForCurrent() #1139

Conversation

M-Zuber
Copy link
Contributor

@M-Zuber M-Zuber commented Mar 10, 2016

Fixes #1136

@ryangribble
Copy link
Contributor

Just me being pedantic, but now the method has been renamed, can we also please rename the unit test classes?

eg class TheGetAllMethod should now be class TheGetAllForCurrentMethod here and here

Also AppVeyor build seems to be throwing a Code Analysis CA1024 error

MSBUILD : error CA1024: Microsoft.Design : Change 'IUserKeysClient.GetAllForCurrent()' to a property if appropriate. [C:\projects\octokit-net\Octokit\Octokit.csproj]

Im not sure why the method name GetAlldidnt trigger it but GetAllForCurrentdoes, but you can suppress it (varoius examples through the codebase like here

@shiftkey
Copy link
Member

can we also please rename the unit test classes?

👍

Im not sure why the method name GetAll didnt trigger it but GetAllForCurrentdoes, but you can suppress it

I forget the exact reasons, but I gather FxCop just doesn't like methods without parameters (even though these return something more complex than a value)...

EDIT: nevermind, MSDN has my back:

A public or protected method has a name that starts with Get, takes no parameters, and returns a value that is not an array.

@ryangribble
Copy link
Contributor

That doesn't explain why it didn't trip the error previously when called GetAll though does it?

@shiftkey
Copy link
Member

@ryangribble it does not, no...

@ryangribble
Copy link
Contributor

Hmm the build failures look like they couldn't talk to nuget.org at the time...

I kicked off the Travis build again and that's now passed. Don't think I have access to do the same on Appveyor @haacked @shiftkey

@shiftkey
Copy link
Member

@ryangribble I'm on it

@shiftkey
Copy link
Member

This all looks good to me. I'll let @ryangribble hit the big green button on it (it won't go out in v0.19, but that's a minor thing).

ryangribble added a commit that referenced this pull request Mar 13, 2016
…-GetAllForCurrent

IUserKeysClient.GetAll -> GetAllForCurrent()
@ryangribble ryangribble merged commit 00cdf99 into octokit:master Mar 13, 2016
@ryangribble
Copy link
Contributor

LGTM

@shiftkey
Copy link
Member

shiftkey commented Apr 8, 2016

release_notes: Renamed IUserKeysClient.GetAll() to IUserKeysClient.GetAllForCurrent()

@ryangribble ryangribble mentioned this pull request Jun 14, 2016
6 tasks
@nickfloyd nickfloyd added Type: Bug Something isn't working as documented and removed category: bug labels Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants