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

Implement protodef-yaml for mcpc #886

Merged
merged 16 commits into from
Jul 18, 2024
Merged

Implement protodef-yaml for mcpc #886

merged 16 commits into from
Jul 18, 2024

Conversation

extremeheat
Copy link
Member

  • Implement protodef-yaml for mcpc
  • Support pc in incrementVersion script

@extremeheat extremeheat marked this pull request as ready for review June 23, 2024 23:22
Fix not updating protocol/version in dataPath for newest version
@rom1504
Copy link
Member

rom1504 commented Jul 2, 2024

it does look pretty good

do you think we can do anything about older versions?

@rom1504
Copy link
Member

rom1504 commented Jul 2, 2024

I think we should update docs for explaining contributors where they need to update the protocol info

@extremeheat extremeheat requested a review from rom1504 July 9, 2024 23:29
@rom1504
Copy link
Member

rom1504 commented Jul 13, 2024

sorry for the delay in reviewing, I've been pretty busy in last months.

This looks pretty good!

One last thing I am noticing is the addition of default void in many places; which seems incorrect to me. For most switches the default is not void. There is no default! In protodef it means it will fail if value is not among the specified choices.
Could you fix that?

Fix protodef-yaml setting switch defaults to void
Copy link
Member

@rom1504 rom1504 left a comment

Choose a reason for hiding this comment

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

Thank you for this large improvement!

@rom1504 rom1504 merged commit a748994 into master Jul 18, 2024
4 checks passed
@rom1504 rom1504 deleted the pc-yaml branch July 18, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants