Adding possiblity to get ssh keys#544
Conversation
bitwiseman
left a comment
bitwiseman
left a comment
There was a problem hiding this comment.
Thanks for contributing!
Solid work with tests.
A few questions/comments, once those are addressed we can merge.
| public class GHUser extends GHPerson { | ||
|
|
||
| public List<GHKey> listKeys() throws IOException { | ||
| return Collections.unmodifiableList(Arrays.asList(root.retrieve().to("/users/"+login+"/keys", GHKey[].class))); |
There was a problem hiding this comment.
It is possible for there to be multiple pages of keys? I know it is unlikely for some one to have more than a page of keys, but if it is possible it might be better to use a PagedIterable.
There was a problem hiding this comment.
I think it is a over kill using PagedIterable, but it is of course possible. Most people only have a single ssh key and less than 10.
Personally I think it better to return a list and instead read all the keys in listKeys() . I can of course iterate until all key have been read
There was a problem hiding this comment.
@arngrimur-seal
I see your point.
Okay, I just did a quick review of the code base. Generally, list* methods return PagedIterable<T>. If there is a method that returns a List<T> or a T[] it is called get*.
I think this completely misleading naming, but I'd rather be consistent with the existing naming pattern. So, let's change the name to getKeys(). Then we're good to go.
There was a problem hiding this comment.
Not sure I understand, are we good to merge or do you want me to update the code so we make sure all keys has been rtrieved?
There was a problem hiding this comment.
Added a suggestion comment above.
| } | ||
|
|
||
| @Test | ||
| public void getKeys() throws IOException { |
There was a problem hiding this comment.
This is really great. How was it using this test framework and takeSnapshot? I just added it and would appreciate any feedback you can offer.
There was a problem hiding this comment.
It took a while before I understood the framework but then it was simple. Deleting and generating the test data has drawbacks since it is easy do forget about that step but the advantage with test taking lots of time and that data is constant is a greater benefit.
There was a problem hiding this comment.
Okay, well, if you have any suggestions for how to make it easier (such as improvements to the CONTRIBUTING.md) that would be great.
| */ | ||
| public class GHUser extends GHPerson { | ||
|
|
||
| public List<GHKey> listKeys() throws IOException { |
There was a problem hiding this comment.
| public List<GHKey> listKeys() throws IOException { | |
| public List<GHKey> getKeys() throws IOException { |
| @Test | ||
| public void getKeys() throws IOException { | ||
| GHUser u = gitHub.getUser("rtyler"); | ||
| List<GHKey> ghKeys = new ArrayList<>(u.listKeys()); |
There was a problem hiding this comment.
| List<GHKey> ghKeys = new ArrayList<>(u.listKeys()); | |
| List<GHKey> ghKeys = new ArrayList<>(u.getKeys()); |
2 participants
No description provided.