- 
                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
      
            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. |