Skip to content
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.

Sign up for GitHub

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

fix!: Refactor the repository ruleset code #3430

Merged
merged 16 commits into from
Jan 21, 2025
+3,791 −4,723
Merged

fix!: Refactor the repository ruleset code #3430

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9c5fc32
fix!: Refactored the repository ruleset code
stevehipwell Jan 13, 2025
a38ea41
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 13, 2025
152169a
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 13, 2025
959df1e
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 14, 2025
a149d40
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 14, 2025
b9041af
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 14, 2025
a24903b
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 14, 2025
b12b08c
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 20, 2025
6971bef
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 20, 2025
1f00cfe
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 20, 2025
528ce0e
Merge branch 'master' into refactor-rules
gmlewis Jan 20, 2025
dffa330
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 21, 2025
81b59ee
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 21, 2025
49fbce8
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 21, 2025
47d0c77
Merge remote-tracking branch 'upstream/master' into refactor-rules
stevehipwell Jan 21, 2025
7241f1a
fixup! fix!: Refactored the repository ruleset code
stevehipwell Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Loading
Loading status checks…
fixup! fix!: Refactored the repository ruleset code 
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
@stevehipwell
stevehipwell committed Jan 21, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
commit 49fbce8c2f7e0e64c40adab502cb8f2c5fbecc3d
9 changes: 4 additions & 5 deletions github/rules.go
View file
Original file line number Diff line number Diff line change
@@ -479,7 +479,8 @@ type repositoryRulesetRuleWrapper struct {

// MarshalJSON is a custom JSON marshaler for RulesetRules.
func (r *RepositoryRulesetRules) MarshalJSON() ([]byte, error) {
// If new rules are added to RulesetRules the capacity needs increasing
// The RepositoryRulesetRules type marshals to between 1 and 21 rules.
// If new rules are added to RepositoryRulesetRules the capacity below needs increasing
rawRules := make([]json.RawMessage, 0, 21)
Copy link
Collaborator

@gmlewis gmlewis Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can you please add a comment here as to why 21 was chosen for the capacity, for the benefit of the maintainers and developers?

If it is completely arbitrary, then please remove it entirely, as that is not idiomatic Go.
The Go team has gone to extensive lengths with benchmarking to make the defaults very reasonable, and if you don't have a good reason for the capacity, then it is not only distracting, but makes the code harder to read and understand, which also violates idiomatic Go, as one of the whole points of Go's design is that it should be super-easy to read and understand.

Therefore, if it is arbitrary, it is idiomatic Go to always take advantage of the zero-value of a type when its initial value is not the focus of the code. Therefore, this would simply be:

  var rawRules []json.RawMessage

Short, sweet, and does not distract the reader into thinking there is something special going on here.
In fact, the comment on 482 is also distracting and reader-time-consuming, in my opinion, if there is nothing special going on here.

However, once again, if the 21 IS important (it is not obvious to me how it possibly could be), then change the comment on 482 to explain why 21 is important. Otherwise, this is just a magic number:
https://en.wikipedia.org/wiki/Magic_number_(programming)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment, but basically there will always be between 1 and 21 items in the slice due to the GitHub data model. My understanding was that if you had prior knowledge on the size of a Go array/slice that you should explicitly set it? If I'm incorrectly informed in this case I'll happily remove the make() call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, fine, then please change the comment. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed 2 of the 3 cases as they were incorrect and improved the comment for the case where I think it's worth setting the capacity.


if r.Creation != nil {
@@ -674,8 +675,7 @@ func marshalRepositoryRulesetRule[T any](t RepositoryRuleType, params T) ([]byte

// UnmarshalJSON is a custom JSON unmarshaler for RulesetRules.
func (r *RepositoryRulesetRules) UnmarshalJSON(data []byte) error {
// If new rules are added to RulesetRules the capacity needs increasing
wrappers := make([]repositoryRulesetRuleWrapper, 0, 21)
var wrappers []repositoryRulesetRuleWrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var wrappers []repositoryRulesetRuleWrapper
var wrappers []*repositoryRulesetRuleWrapper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed now.


if err := json.Unmarshal(data, &wrappers); err != nil {
return err
@@ -836,8 +836,7 @@ type branchRuleWrapper struct {

// UnmarshalJSON is a custom JSON unmarshaler for BranchRules.
func (r *BranchRules) UnmarshalJSON(data []byte) error {
// If new rules are added to RulesetRules the capacity needs increasing
wrappers := make([]branchRuleWrapper, 0, 21)
var wrappers []branchRuleWrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var wrappers []branchRuleWrapper
var wrappers []*branchRuleWrapper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed now.


if err := json.Unmarshal(data, &wrappers); err != nil {
return err