-
Notifications
You must be signed in to change notification settings - Fork 726
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
Added getUserPublicOrganizations method #510
Conversation
import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE; | ||
import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED; | ||
import static java.util.logging.Level.FINE; | ||
import static org.kohsuke.github.Previews.DRAX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorted & alphabetized import statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were alphabetical before. Please revert the regrouping. There should be a setting in your IDE to not do this (or use the same sorting as the project already uses).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the setting that you used? (This happens pretty often.)
@@ -172,6 +175,7 @@ public static GitHub connect() throws IOException { | |||
* @deprecated | |||
* Use {@link #connectToEnterpriseWithOAuth(String, String, String)} | |||
*/ | |||
@Deprecated | |||
public static GitHub connectToEnterprise(String apiUrl, String oauthAccessToken) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added @Deprecated
annotation to match @deprecated
javadoc; this is best-practice but may start causing IDE or compile warnings for existing users of the methods.
If the method is truly deprecated, this is fine.
@@ -486,6 +492,7 @@ public GHRepository getRepository(String name) throws IOException { | |||
@Preview @Deprecated | |||
public PagedIterable<GHLicense> listLicenses() throws IOException { | |||
return new PagedIterable<GHLicense>() { | |||
@Override | |||
public PagedIterator<GHLicense> _iterator(int pageSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added @Override
annotation to overridden methods as a matter of lint/best-practice.
* Alias for {@link #getUserPublicOrganizations(String)}. | ||
*/ | ||
public Map<String, GHOrganization> getUserPublicOrganizations(GHUser user) throws IOException { | ||
return getUserPublicOrganizations( user.getLogin() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One method takes a GH*
object, here, a GHUser
.
* | ||
* @return the public Organization memberships for the user | ||
*/ | ||
public Map<String, GHOrganization> getUserPublicOrganizations(String login) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other method takes a plain old String
, so that users' code doesn't have to construct a GHUser
just to look up organizations.
* @return the public Organization memberships for the user | ||
*/ | ||
public Map<String, GHOrganization> getUserPublicOrganizations(String login) throws IOException { | ||
GHOrganization[] orgs = retrieve().to("/users/" + login + "/orgs", GHOrganization[].class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Documentation: https://developer.github.com/v3/orgs/#list-user-organizations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, could you all the override and other tweaks in a separate commit for clarity.
Also, revert the import sorting - the order is already as it should be for this project.
Done. |
…emberships of any user, not just the currently-authenticated one.
@awittha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this looks reasonable. We just need tests in order to merge it.
@awittha Do you think you'll have time to do this in the next week? |
Sorry, it had dropped off my radar. It's already Wednesday, but I'll keep an eye on it and see what I can get done by the end of the week. |
07a41e5
to
c763c02
Compare
@bitwiseman , wrote some tests! |
I didn't see your note about instructions in CONTRIBUTING.md, until after I'd tried & figured out the mocking framework by reading the new code... It seems to work well and do what I need. Didn't get to test the instructions, though. |
It may be the case (especially with GitHub Enterprise installations) that a single authenticated user wants to find out about Organization membership for users other than themselves.
This adds a
getUserPublicOrganizations(GHUser user)
method to enable that lookup.I wasn't able to complete the test-suite locally due to permission issues that sounded similar to this mailing-list post from August 2018.