Skip to content

Conversation

@Kybxd
Copy link
Collaborator

@Kybxd Kybxd commented Apr 23, 2025

@codecov
Copy link

codecov bot commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 92.68293% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.25%. Comparing base (745037b) to head (22db647).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
internal/protogen/exporter.go 91.89% 0 Missing and 3 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

}

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 {
Copy link
Member

@wenchy wenchy Apr 25, 2025

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@Kybxd Kybxd force-pushed the protogen.number-prop branch from c4cba17 to cc3f214 Compare April 25, 2025 03:29
// 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.
Copy link
Collaborator Author

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?

Copy link
Member

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;

Copy link
Collaborator Author

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.

Comment on lines 31 to 33
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}}];
Copy link
Member

Choose a reason for hiding this comment

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

  1. Should remove the prop number:10 as it is already used to specify tag id, no useless to keep it.
  2. 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. done
  2. a new algorithm is introduced to guarantee auto generated field numbers do not conflict with specified ones.

// 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.
Copy link
Member

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;

@Kybxd Kybxd force-pushed the protogen.number-prop branch from cc3f214 to 9fd30b5 Compare May 30, 2025 09:37
@wenchy
Copy link
Member

wenchy commented Jul 8, 2025

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:

  1. Auto keep field number by unique column name.
  2. If new field name occured, then assign the the max field number plus 1 in the same level.

This can avoid field number overwriting for different column names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

protogen: guarantee message field's Tag Number compatibility

3 participants