-
Notifications
You must be signed in to change notification settings - Fork 8
protogen: add field number prop #230
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
base: master
Are you sure you want to change the base?
Conversation
Kybxd
commented
Apr 23, 2025
- close protogen: guarantee message field's Tag Number compatibility #218
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #230 +/- ##
==========================================
+ Coverage 71.15% 71.25% +0.09%
==========================================
Files 80 81 +1
Lines 10293 10360 +67
==========================================
+ Hits 7324 7382 +58
- Misses 2402 2411 +9
Partials 567 567 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
internal/protogen/exporter.go
Outdated
| } | ||
|
|
||
| func (x *sheetExporter) exportField(depth int, tagid int, field *internalpb.Field, prefix string) error { | ||
| func (x *sheetExporter) exportField(depth int, tagid *int, field *internalpb.Field, prefix string) error { |
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.
For Go, In/Out parameter reuse is confusing, error-prone, and not recommended.
Please return the next tagid instead if you want to return the next one (just like cursor).
exportField(depth int, tagid int, field *internalpb.Field, prefix string) (int, error)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.
done
c4cba17 to
cc3f214
Compare
proto/tableau/protobuf/tableau.proto
Outdated
| // All fields with number unspecified will use a sequence starting from 1 by default, | ||
| // and users should guarantee this number do not conflict with the default sequence. | ||
| // | ||
| // For tables, assigning number to a cross cell map/list/struct field also works on its 1st sub-field. |
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.
Is this acceptable, or we may find another way to distinguish the number prop works on the list/map, or the first subfield of the list/map?
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 think we'd better distinguish them. Add new prop
subnumber(just likesubsep) to specify first sub-field's number of the list/map element. - Also need to improve comment below.
// Specify field number (unique numbered tag).
// All fields without numbers specified will use a sequence starting from 1 by default.
// Besides, auto generated field numbers will not reuse explicitly specified field numbers.
//
// Refer to [protobuf: Assigning Field Numbers](https://protobuf.dev/programming-guides/proto3/#assigning)
int32 number = 16;
// Specify first sub-field's number of the list-element/map-value/struct.
int32 subnumber = 17;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.
We fixed the field number of message's first field to 1. So it's no need to consider subnumber now.
| int32 price = 10 [(tableau.field) = {name:"Price" prop:{number:10}}]; | ||
| int32 number = 3 [(tableau.field) = {name:"Number"}]; | ||
| Decompose decompose = 20 [(tableau.field) = {name:"Decompose" prop:{number:20}}]; |
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.
- Should remove the prop
number:10as it is already used to specify tag id, no useless to keep it. - If specified prop
number, then the other auto generated tag id MUST not use it again in the same nesting level. Add test case to cover 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.
- done
- a new algorithm is introduced to guarantee auto generated field numbers do not conflict with specified ones.
proto/tableau/protobuf/tableau.proto
Outdated
| // All fields with number unspecified will use a sequence starting from 1 by default, | ||
| // and users should guarantee this number do not conflict with the default sequence. | ||
| // | ||
| // For tables, assigning number to a cross cell map/list/struct field also works on its 1st sub-field. |
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 think we'd better distinguish them. Add new prop
subnumber(just likesubsep) to specify first sub-field's number of the list/map element. - Also need to improve comment below.
// Specify field number (unique numbered tag).
// All fields without numbers specified will use a sequence starting from 1 by default.
// Besides, auto generated field numbers will not reuse explicitly specified field numbers.
//
// Refer to [protobuf: Assigning Field Numbers](https://protobuf.dev/programming-guides/proto3/#assigning)
int32 number = 16;
// Specify first sub-field's number of the list-element/map-value/struct.
int32 subnumber = 17;cc3f214 to
9fd30b5
Compare
|
It's too verbose to specify field number for each column. Another solution is automation (auto keep compatibilty), when compared with the old protoconf file:
This can avoid field number overwriting for different column names. |