Skip to content
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

Transparent encoding #91

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Transparent encoding #91

merged 4 commits into from
Feb 2, 2024

Conversation

magik6k
Copy link
Collaborator

@magik6k magik6k commented Jan 24, 2024

This PR adds support for 'transparent' encoding, either in structs with cborgen:"transparent" tag (which can be used with other tags), or by just defining a newtype that isn't a struct

type IntAlias int64

type IntArray struct {
	Ints []int64 `cborgen:"transparent"`
}
type IntAliasArray struct {
	Ints []IntAlias `cborgen:"transparent"`
}

type IntArrayNewType []int64

type IntArrayAliasNewType []IntAlias

type MapTransparentType map[string]string

Opinions this is based on:

  • Transparent encoders can only be generated in tuple mode
    • Map mode entries have name prefixes, so technically tuple mode makes more sense
  • Only one transparent entry in structs with transparent entries
    • Technically we could allow multiple entries, in any case we're just stripping the array header, but that will make things like encoding structs with multiple transparent entries in maps/arrays much more complicated, so just one transparent entry for now

@Stebalien
Copy link
Collaborator

Do we need the version that supports the transparent tag? Personally:

  1. I'd prefer to just stick with newtypes.
  2. I'm not a huge fan of using a tag here, I'd consider introducing a WriteTransparentEncodersToFile. On the other hand, it is nice having this information in the file itself.

@Stebalien
Copy link
Collaborator

(basically, I'd consider just sticking with newtypes to avoid the bikeshed)

gen.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Contributor

rvagg commented Jan 24, 2024

Would be nice to see tests with pre-recorded forms rather than just roundtrips. I'd come up with some but I'm only on my phone at the moment. If you npm install cborg -g then you can do things like cborg json2hex '["foo",[1,2,3],true]'.

gen.go Outdated Show resolved Hide resolved
gen.go Outdated Show resolved Hide resolved
gen.go Outdated Show resolved Hide resolved
@magik6k
Copy link
Collaborator Author

magik6k commented Jan 24, 2024

Do we need the version that supports the transparent tag?

I’d say yes, this is the only way that makes it possible to do things like set array size limits and nil slice handling, which will inevitably be useful

@Stebalien
Copy link
Collaborator

Ah, good point.

Copy link
Collaborator

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

This looks correct. I'd much prefer to find a way to split this out into multiple functions as I think someone will trip over this eventually, but I'm not the one writing the code so I'm not sure how much work that would be.

Copy link
Contributor

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

nice

Copy link
Owner

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Neat, seems useful

@whyrusleeping whyrusleeping merged commit fcdaca4 into master Feb 2, 2024
@Stebalien Stebalien deleted the feat/transparent-encoding branch February 2, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants