-
Notifications
You must be signed in to change notification settings - Fork 97
[STAR-789] Add proposed Protection Level message #737
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
Conversation
8e4498f to
b08a501
Compare
lloydmaza
left a comment
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.
Message definition basically looks good to me, besides a couple of small nits. Just make sure to update the message description before merging anything.
15242b2 to
f14b46f
Compare
scarcanague
left a comment
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.
A few comments as per @denniszollo suggestions
130b747 to
688b9a7
Compare
lloydmaza
left a comment
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.
The changes I advised have been implemented, so I'm glad to give this the green check.
It would be good to gather feedback from some other voices to see if the changes to the message definition are satisfactory for this initial implementation.
scarcanague
left a comment
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.
1 final comment but looks good otherwise
688b9a7 to
68f9c92
Compare
Split this into two commits. The first contains the update to the spec, the second contains all of the regenerated code.