Add getUser method and User.AvatarUrl property #4
Conversation
Git.hub/Client.cs
Outdated
| @@ -148,6 +148,20 @@ public User getCurrentUser() | |||
| return user.Data; | |||
| } | |||
|
|
|||
| public User getUser(string username) | |||
There was a problem hiding this comment.
The method must be in PascalCase (e.g. GetUser) and arguments must be in camelCase (e.g. userName)
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions
There was a problem hiding this comment.
I would usually agree, but everything else in this class follows camelCase, so I did it too... I can change it if you want, but then it will be inconsistent. I'll fix the param name, though.
There was a problem hiding this comment.
I haven't actually looked at the existing code at all.
CamelCase is soooo Javaish and not C#ish
There was a problem hiding this comment.
Yeah, I know... But the current code is like this. So, which is worse? Using camelCase, or having inconsistent naming? I don't mind either way, just let me know ;)
There was a problem hiding this comment.
I've looked through the codebase and for the most part it looks to adhere to naming standards. This class is an odd ball.
So if you would please align you code with the MS naming standards.
We can get the rest fixed separately.
Git.hub/Client.cs
Outdated
| throw new ArgumentNullException(nameof(username)); | ||
|
|
||
| if (username.Length == 0) | ||
| throw new ArgumentException("Empty username", nameof(username)); |
There was a problem hiding this comment.
Combine both checks into one
if (string.IsNullOrWhiteSpace(userName))
{
throw new ArgumentException("Empty username", nameof(userName));
}
There was a problem hiding this comment.
I made separate checks because I thought it would be better to throw ArgumentNullException for a null argument... I don't mind changing it if you prefer a single check with a less appropriate exception.
There was a problem hiding this comment.
Less code the better :)
IMO two checks don't really provide any additional value in this case.
Git.hub/Client.cs
Outdated
| var request = new RestRequest($"/users/{username}"); | ||
|
|
||
| var user = client.Get<User>(request); | ||
| return user.Data; |
There was a problem hiding this comment.
Should the method handle exceptions (e.g. connection timeout, host not found etc, 404)?
There was a problem hiding this comment.
I tried to follow existing code patterns. The getCurrentUser method only checks for null and assumes it's because of bad credentials (although it could in fact be anything). But this API doesn't require authentication, so I removed this check. If you like, I could throw a "Not found" exception if the user doesn't exist, but just returning null makes sense too. As for connection timeout, etc, the other methods don't do it, so I didn't think it would make sense to do it here.
There was a problem hiding this comment.
It doesn't seem the existing code makes itself concerned with error handling. So I suppose you just follow the existing patterns.
That is something we'll have to tackle separately
|
I'll trawl through the code when I have time and come back to you
…On 14/11/2017 8:32 AM, "Thomas Levesque" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Git.hub/Client.cs
<#4 (comment)>:
> @@ -148,6 +148,20 @@ public User getCurrentUser()
return user.Data;
}
+ public User getUser(string username)
Yeah, I know... But the current code is like this. So, which is worse?
Using camelCase, or having inconsistent naming? I don't mind either way,
just let me know ;)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#4 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEMyXntNltlBs8sVX40xlcf0USfcbCBeks5s2LVpgaJpZM4QcRFW>
.
|
thomaslevesque
force-pushed
the
get-user-and-avatar
branch
from
25627f9 to
d936886
Compare
|
I made a fixup commit. Let me know if you want me to squash. |
|
Yes, please squash
…On 15 November 2017 at 22:28, Thomas Levesque ***@***.***> wrote:
I made a fixup commit. Let me know if you want me to squash.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#4 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEMyXoW5gkcmbrM1L2KEYVS7I6BmImM-ks5s2srWgaJpZM4QcRFW>
.
|
thomaslevesque
force-pushed
the
get-user-and-avatar
branch
from
d936886 to
245761c
Compare
|
Done |
|
Awesome
So how is it going to get used? Are there more changes coming?
…On 15 November 2017 at 23:18, Thomas Levesque ***@***.***> wrote:
Done
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#4 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEMyXhXCAV2uHdeouY6QvhAJsvrOFA30ks5s2tZ4gaJpZM4QcRFW>
.
|
Yes, in the GitExtensions repo. I have a POC ready, but it will probably need some refactoring. |
In preparation for gitextensions/gitextensions#3770