-
Notifications
You must be signed in to change notification settings - Fork 26
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
Introduce Gen{} struct for configurability #94
Conversation
One minor problem is that |
@@ -1255,7 +1302,7 @@ func emitCborUnmarshalSliceField(w io.Writer, f Field) error { | |||
Pointer: pointer, | |||
Name: f.Name + "[" + f.IterLabel + "]", | |||
} | |||
err := emitCborUnmarshalStructField(w, subf) | |||
err := g.emitCborUnmarshalStructField(w, subf) |
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.
Leave it for now, but we can probably fold the writer into Gen
in the future.
MaxArrayLength: 10, | ||
MaxByteLength: 9, | ||
MaxStringLength: 8, | ||
}.WriteTupleEncodersToFile("testing/cbor_options_gen.go", "testing", |
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.
Any reason not to change this to:
err := cbg.Gen{
MaxArrayLength: 10,
MaxByteLength: 9,
MaxStringLength: 8,
TupleStructs: []any{
types.LimitedStruct{},
},
TransparentTypes: ...
}.WriteToFile("testing/ccbor_options_gen.go", "testing")
I ask because this would let easily use different representations for different types.
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.
no reason other than wanting to keep the scope of this minimal; I was tempted to deprecate the Write{Type}*
functions and do this kind of thing. Although in my version I was going to go with a TupleStruct bool
in the config, but moving the types list also into the config could also work.
maybe we should hold off on this for a future change? Because at least with the changes now it's all backward compatible and doesn't require anyone to change anything except if they want different defaults.
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 could make it backwards compatible, but not cleanly because the function specifies the encoding. My idea was to have separate fields (TupleStructs
, TransparentTypes
, MapStructs
, etc.) for different encodings so we can put them all in the same file.
Alternatively, we can just have multiple files. That's really not an issue, honestly.
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'm not a huge fan of mixing "tuple encoding" and "transparent types" as we do now as they're different encodings)
type Gen struct { | ||
MaxArrayLength int // Default: 8192 (MaxLength) | ||
MaxByteLength int // Default: 2<<20 (ByteArrayMaxLen) | ||
MaxStringLength int // Default: 8192 (MaxLength) |
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.
Was this actually 8192 before? That's... ok, I guess. But strange.
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.
yeah, strange, but also not heavily used in filecoin, so not surprising that it hasn't come up before
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.
yeah, was just some number that got picked as 'probably big enough for a default'
@@ -37,9 +66,18 @@ func doTemplate(w io.Writer, info interface{}, templ string) error { | |||
"ReadHeader": func(rdr string) string { | |||
return fmt.Sprintf(`%s.ReadHeader()`, rdr) | |||
}, | |||
"MaxLen": func(val int, def string) string { | |||
"MaxLen": func(val int, defaultType string) string { |
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.
In retrospect, we probably could have just gone with a number (enum) for defaultType
.
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.
Yeah, but this comes in from a template so we'd end up having to use integers there which is more opaque {{ MaxLen .MaxLen "String" }}
-> {{ MaxLen .MaxLen 2 }}
. Unless we share them as functions that take no arguments, but then we'd have to pipe: {{ String | MaxLen .MaxLen }}
. Unless I'm missing a template feature that could be used here?
Oh! Thats annoying, i'm sorry for that. |
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.
This looks great, Thank you @rvagg ! And really sorry about breaking downstreams on the update, forgot about how annoying turning an untyped const into a typed var is...
Ref: #92
Reverts #92 because the breakage is very ouchy.
Just starting with the two main globals, but this does open up some interesting possibilities!