-
Notifications
You must be signed in to change notification settings - Fork 173
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
refactor mstatus handler function #652
base: master
Are you sure you want to change the base?
Conversation
8c3ba10
to
0953d93
Compare
@Timmmm Is there anything else that needs improvement in my code? |
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 - would be nice to get a second opinion though. Any takers @arichardson @jordancarlin etc?
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.
This looks like an improvement to me. Nit: not sure we need to add all the future fields in this PR but I don't mind keeping them 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.
Minor nit in a comment but otherwise LGTM
0953d93
to
8633212
Compare
As mentioned in #639, and there are also some legitimacy issues in legalize_mstatus, so this PR is intended to help make corrections