-
Notifications
You must be signed in to change notification settings - Fork 0
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 formAdmin struct #4
Conversation
contracts/evoting/json/adminForm.go
Outdated
return nil, xerrors.Errorf("failed to marshal form: %v", err) | ||
} | ||
|
||
return buff, nil |
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 take it the idea behind the switch
statement is to add other formats at a later point?
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 just took the same coding style/convention of the equivalent file for the "classic" form: https://github.com/MaximeZmt/d-voting/blob/main/contracts/evoting/json/forms.go
Should I change it ?
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.
if there's only one format the switch
logic seems to be a bit superfluous to me, especially as you're not doing it in the Decode
function - I'd just do something like this:
func (adminFormFormat) Encode(ctx serde.Context, message serde.Message) ([]byte, error) {
if message.(type) != types.AdminForm {
return nil, xerrors.Errorf("Unknown format: %T", message)
}
<rest of the function>
}
like this it returns immediately if it's not the correct type (no need for an else
which makes it more readable), and the logic is simpler (makes it easier to understand)
switch
makes only sense if there's multiple cases
edit .gitignore to add new line at the end as before
return buff, nil | ||
default: | ||
adminForm, ok := message.(types.AdminForm) | ||
if !ok { |
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.
if there is no error it is the correct format?
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.
LGTM
Thank you for opening a pull request with this project, please also:
draft
mode andin progress
pipeline in the project (if applicable)Review
orReady4Merge
pipeline in the project (if applicable)draft