Skip to content

Conversation

@roosre
Copy link
Contributor

@roosre roosre commented Nov 16, 2022

No description provided.

@roosre roosre requested a review from greschd November 16, 2022 07:50
Copy link
Member

@greschd greschd left a comment

Choose a reason for hiding this comment

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

Thanks @roosre, just one question.

@@ -0,0 +1,15 @@
syntax = "proto3";
Copy link
Member

Choose a reason for hiding this comment

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

Are we using this in different objects, or could it be defined directly in the materials.proto?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ply type is only used by the material, at least as defining property. It is used in several places in the current UI though (ESAComp export, upgrade Mechanism). Do you prefer to move it to materials.proto?

Choose a reason for hiding this comment

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

As you asked me to look at the PRs, I'll comment as well:
Looks like you define a new enum here, and there is already something called "enum_types.proto", why not add it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name of the values have to be unique per namespace and I thought that UNKNOWN is used for the Status and PlyType but this is not the case.

Copy link

@ojkoenig ojkoenig Nov 17, 2022

Choose a reason for hiding this comment

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

Was curious about the scopes of enums for protobuf, so I quickly googled, finding this:

Here they say in C++ it maps to C++ scoping rules, meaning you cannot have two enums with same values.
https://protobuf.narkive.com/6BXBO4QR/enum-values-are-siblings-of-their-type-not-children-of-it

This is probably why the protobuf doc here uses unique enum values with 'enum_name'_'value' :
https://developers.google.com/protocol-buffers/docs/proto3#enum
?

@roosre roosre requested a review from ojkoenig November 17, 2022 09:56
Copy link

@ojkoenig ojkoenig left a comment

Choose a reason for hiding this comment

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

General question:
From this PR and the one on ACP side it looks a bit look you keep the proto defs manually in sync in acp kernel and pyacp repo? I would expect that protos are defined in one place and then be consumed as some sort of pkg/zip on the other end?

@@ -0,0 +1,15 @@
syntax = "proto3";

Choose a reason for hiding this comment

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

As you asked me to look at the PRs, I'll comment as well:
Looks like you define a new enum here, and there is already something called "enum_types.proto", why not add it there?

@roosre roosre merged commit 066950f into main Nov 17, 2022
@roosre roosre deleted the rroos/support_dpf_composites branch November 17, 2022 12:07
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.

4 participants