Skip to content

Update ProtectionChanges to contain the remaining possible return values - #2486

Merged
gmlewis merged 4 commits into
google:masterfrom
eolso:update-branch-protection-changes
Oct 4, 2022
Merged

Update ProtectionChanges to contain the remaining possible return values#2486
gmlewis merged 4 commits into
google:masterfrom
eolso:update-branch-protection-changes

Conversation

@eolso

@eolsoeolso commented Oct 4, 2022

Copy link
Copy Markdown
Contributor

Fixes issue #2485. The From values only seem to come in the variants of string, bool or []string, so it's possible these objects could be consolidated into something more generic maybe.
e.g.

typeFromStringstruct {
From*string`json:"from,omitempty"`
}
typeFromBoolstruct {
From*bool`json:"from,omitempty"`
}
typeFromStringSlicestruct {
From []string`json:"from,omitempty"`
}

This went against the current pattern so I made everything a <Value>Changes struct instead :D

@google-cla

google-claBot commented Oct 4, 2022

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@codecov

codecovBot commented Oct 4, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2486 (e7143e7) into master (08819b7) will not change coverage.
The diff coverage is n/a.

@@ Coverage Diff @@## master #2486 +/- ##
=======================================
Coverage 98.03% 98.03% =======================================
Files 122 122 Lines 10705 10705 =======================================
Hits 10495 10495 Misses 144 144 Partials 66 66 
Impacted FilesCoverage Δ
github/repos.go98.67% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @eolso !
Just a few minor tweaks, please.

Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
Comment threadgithub/repos.go Outdated
@gmlewisgmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Oct 4, 2022
@eolso

eolso commented Oct 4, 2022

Copy link
Copy Markdown
ContributorAuthor

Fixed!

@gmlewis the structs for the fields AuthorizedActorNames and AuthorizedActorsOnly could be left alone to prevent any breaking changes if that's preferred? I only renamed them to keep the pattern consistent in the struct. I also only added the Changes to the end of each type name because RequiredStatusChecks was already defined elsewhere 😅

@gmlewis

Copy link
Copy Markdown
Collaborator

Fixed!

@gmlewis the structs for the fields AuthorizedActorNames and AuthorizedActorsOnly could be left alone to prevent any breaking changes if that's preferred? I only renamed them to keep the pattern consistent in the struct. I also only added the Changes to the end of each type name because RequiredStatusChecks was already defined elsewhere sweat_smile

Thank you, @eolso - I'll review this after work.

Meanwhile, yes, I prefer to avoid breaking API changes if possible... and in this case I think it is probably fine to have the slight inconsistencies... so maybe reverting these two name changes would be preferable. If any other contributors have other opinions, I'm happy to discuss it.

@eolso

eolso commented Oct 4, 2022

Copy link
Copy Markdown
ContributorAuthor

Agreed, "consistency" can wait. Reverted breaking changes 😄

@gmlewisgmlewis removed the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Oct 4, 2022

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @eolso !
LGTM.
Merging.

@gmlewis
gmlewis merged commit 330d92d into google:masterOct 4, 2022
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@eolso@gmlewis