Conversation
feat: add xxxFields for some struct
|
@kepinsu ready for review or do you still have anything to update? |
|
Yes, it's ready for review |
|
@wmnsk i put 2 commits, the first to update (it's my fault i forgot it...) the list of supported IEs in the readme.MD. The last, i changed (more exactly optimize) the way to retrieve the list of IEs contained (for createFARFields) |
There was a problem hiding this comment.
Great work, but I think we need to discuss before actually implementing this:
The problem of defining the fields with built-in types is nil checking - we will no longer be able to differentiate if it is absent in the packet or it does but with empty value ("", 0, false, etc.), which wasn't discussed in the original ticket.
One way of avoiding this is to define all the fields as ie.IE and let users call the function with the same name (like what we do in the message structure.
Currently, we can get values like this. It's simple enough, but under the water, we iterate over the IEs at each line, so the more fields a user needs to access, the more expensive it gets.
// s = SessionEstablishmentRequest
s.CreateFAR.FARID()
s.CreateFAR.BARID()
// or like this;
createFAR := s.CreateFAR.CreateFAR()
createFAR.FARID()
createFAR.BARID()Your implementation makes it more efficient, but as mentioned above, we can't tell the absence and initial value.
createFAR := s.CreateFAR.CreateFAR()
// both can be 0 if absent or it is actually 0
createFAR.FARID
createFAR.BARIDThe suggested way (define every field as ie.IE type would make it look like this (not very much more useful/efficient than current implementation though);
createFAR := s.CreateFAR.CreateFAR()
createFAR.FARID.FARID()
createFAR.BARID.BARID()
// they can be nil-checked
if createFAR.FARID != nil { ... }IMO, I'm inclined to keep the current implementation or defining with ie.IE. Users can still make it efficiently by defining an iterator over IEs that only retrieves the IEs that they need instead of using functions we provide (or they can choose to be inefficient but convenient).
Please check how you actually make use of the library-provided functions in your app, and let me know your thoughts.
|
|
||
| // Parse all IES heres |
There was a problem hiding this comment.
| // Parse all IES heres |
typo, but in the first place I don't think we need this comment, as it's obvious
| if i.Type != AccessAvailabilityReport { | ||
| return nil, &InvalidTypeError{Type: i.Type} | ||
| } | ||
| // Check if the ie.Parse have called or not |
There was a problem hiding this comment.
In which kind of situation it is already parsed and it is not? Clarifying that in the comment would definitely help devs in the future.
| RequestedAccessAvailabilityInformation uint8 | ||
| } | ||
|
|
||
| func ParseAccessAvailabilityControlInformationFields(b []byte) (*AccessAvailabilityControlInformationFields, error) { |
| return c, nil | ||
| } | ||
|
|
||
| // ParseIEs will iterator over all childs IE to avoid to use Parse or ParseMultiIEs any time we iterate in IE |
There was a problem hiding this comment.
| // ParseIEs will iterator over all childs IE to avoid to use Parse or ParseMultiIEs any time we iterate in IE | |
| // ParseIEs will iterate over all childs IE to avoid to use Parse or ParseMultiIEs any time we iterate in IE |
typo, but is it necessary for this function to be exported?
| if len(ies.ActivatePredefinedRules) > 0 { | ||
| return ies.ActivatePredefinedRules, nil |
There was a problem hiding this comment.
We return "" by default, so this check doesn't really do nothing. See the top-level comment for nil vs empty value handling
| a := &AccessAvailabilityControlInformationFields{} | ||
| for _, ie := range ies { | ||
| if ie == nil { | ||
| continue |
There was a problem hiding this comment.
In general, each XxxFields structure should have IEs or ExtraIEs that contains all the IEs that did not match the predefined types. This helps users access the IEs that we don't explicitly support (e.g., those added in the next spec update won't necessarily be added in structure in real-time).
|
Thank you for your feedback. You're right, I had written it for tracing but had overlooked the fact that users don't necessarily need all the fields (I was focused on my specific need at that moment). I'll instead do it as you suggested. type xxxFields{
xxx *ie.IE
}
// Validate can be used to check if all mandatory fields is present or not
func (x *xxxFields) Validate() error{}And I'll create it in a separate library that fits my needs (since I now also need to iterate over the fields without having all of them). |
2 participants
Add xxxFields Features (it's very painful)
It's a pull request for #124
Group IE
Here is the list of IEs defined in Table 7.x instead of Clause 8.x in TS 29.244 (horewer, i don't implement any method to pass xxxFields to ie.IE object)
Simple IE
Unfinished IE