-
Notifications
You must be signed in to change notification settings - Fork 741
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
Provide support for External Group operations #1835
Provide support for External Group operations #1835
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1835 +/- ##
============================================
+ Coverage 80.64% 81.03% +0.38%
- Complexity 2358 2409 +51
============================================
Files 225 233 +8
Lines 7203 7344 +141
Branches 395 399 +4
============================================
+ Hits 5809 5951 +142
+ Misses 1149 1148 -1
Partials 245 245 ☔ View full report in Codecov by Sentry. |
adad47f
to
934f72a
Compare
|
934f72a
to
8c859c0
Compare
@bitwiseman, is anything else required on my side before continuing the contribution process? |
@bitwiseman Please take a moment to review this Pull Request. It contains important changes that could significantly improve the GitHub Enterprise accounts use. If you notice any issues or have any concerns, do not hesitate to comment. Your feedback is greatly appreciated. |
* | ||
* @author Miguel Esteban Gutiérrez | ||
*/ | ||
public class GHExternalGroups { |
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.
Modify this to match
https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GHCommitFilesPage.java
Rename to GHExternalGroupsPage
Make groups
an array.
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. I don't like handling arrays, but understand that is a particular pattern used across the client.
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 your change shows is that maybe we don't need to use arrays. Could you file an issue for this?
src/test/java/org/kohsuke/github/EnterpriseManagedSupportTest.java
Outdated
Show resolved
Hide resolved
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.
Very well tested and implemented. Great job!
A few adjustments and renames needed.
8fef802
to
e43c5a9
Compare
@bitwiseman, first of all, thanks for the review! I've done all the requested changes, addressing each of them in its own commit. I've also answered to all the conversations, but haven't resolved them. I let that to you in Again, thanks for your effort and patience! |
e43c5a9
to
a363a04
Compare
Rebased from main again. |
Description
Provide support for External Group operations, in particular:
Fixes #1828
Before submitting a PR:
@link
JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .mvn -D enable-ci clean install site
locally. If this command doesn't succeed, your change will not pass CI.main
. You will create your PR from that branch.When creating a PR: