-
Notifications
You must be signed in to change notification settings - Fork 723
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
Add functionality to get active rules for a branch #1897
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1897 +/- ##
============================================
+ Coverage 81.05% 81.29% +0.24%
- Complexity 2442 2450 +8
============================================
Files 237 238 +1
Lines 7342 7449 +107
Branches 398 400 +2
============================================
+ Hits 5951 6056 +105
- Misses 1145 1146 +1
- Partials 246 247 +1 ☔ View full report in Codecov by Sentry. |
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.
I like the idea you have going with Parameter<T>
and the instances defined in the Parameters
interface, but I'm not sure it's the right way to go.
I noted a bunch of style and detail changes. You can choose to do them or hold off until I wrap my head around the Parameter
design and figure out how I feel about it.
Co-authored-by: Liam Newman <bitwiseman@gmail.com>
@bitwiseman I will take care of your suggestions. As of the Parameter vs. Polymorphism topic I had to thoughts about it as well. The parameter way turned out to be more flexible, if GitHub does any change to the API (not sure, if that would happen with the current version being almost two years old). Using polymorphism would result in weird Java code with |
* update_allows_fetch_and_merge parameter | ||
*/ | ||
public static final BooleanParameter UPDATE_ALLOWS_FETCH_AND_MERGE = new BooleanParameter( | ||
"update_allows_fetch_and_merge"); |
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.
Which value of the GHRepositoryRule.Type
enum applies here? Is it suppose to be REQUIRED_LINEAR_HISTORY
?
I'm fine with Parameter<T>
taking string in the constructor, but in Parameters
I want those strings to be generated from GHRepositoryRule.Type
to reduce the likelihood of the strings here getting out of sync with the enum values.
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.
I am not sure, what to do... Enum values cannot have static fields if you think about sth like Type.REQUIRED_LINEAR_HISTORY.UPDATE_ALLOWS_FETCH_AND_MERGE
.
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.
@bitwiseman what should I do here?
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.
Thanks for sticking with this through multiple rounds of feedback. This is looking great, I think this is the last cycle and then we'll be able to merge.
Description
Add methods to get all active rules for a branch.
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: