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.

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

Decode push options in update request #968

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thegrumpylion
Copy link

No description provided.

Signed-off-by: Nikolas Sepos <nikolas.sepos@gmail.com>
@thegrumpylion thegrumpylion force-pushed the decode-push-options-update-request branch from 0c726c2 to b353ec2 Compare March 2, 2024 17:38
@pjbgf pjbgf added this to the v5.12.0 milestone Mar 4, 2024
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

A few nits, otherwise LGTM.

Comment on lines +78 to +82
func errInvalidPushOption(err error) error {
return errMalformedRequest(fmt.Sprintf(
"invalid push option: %s", err.Error()))
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func errInvalidPushOption(err error) error {
return errMalformedRequest(fmt.Sprintf(
"invalid push option: %s", err.Error()))
}

return &Option{Key: string(b)}, nil
}
if i == 0 {
return nil, errInvalidPushOption(errors.New("empty option key"))
Copy link
Member

Choose a reason for hiding this comment

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

Let's create an ErrInvalidPushOption var at the top of the file:

ErrInvalidPushOption = errors.New("invalid push option")
Suggested change
return nil, errInvalidPushOption(errors.New("empty option key"))
return nil, ErrInvalidPushOption

Sign up for free to 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.

None yet

2 participants