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

handle nil slices correctly #88

Merged
merged 4 commits into from
Jan 9, 2024
Merged

handle nil slices correctly #88

merged 4 commits into from
Jan 9, 2024

Conversation

whyrusleeping
Copy link
Owner

This now distinguishes (and correctly encodes) nil slices from zero length slices.
As a result I also had to separate the handling of array types and slice types intro separate generators, since non-pointer array types are not niladic.

@Stebalien
Copy link
Collaborator

This is definitely more correct and does what encoding/json does, but it has a high potential to break downstream projects.

Maybe make this optional? A flag when generating the code and/or a tag on the struct field?

@whyrusleeping
Copy link
Owner Author

Yeah, thats a good idea (and what i was worried about)

@whyrusleeping
Copy link
Owner Author

for now i added a preservenil struct tag to enable this functionality on specific fields. That should unblock what i need this fix for without forcing us to deal with the potential breakages in other things

return xerrors.Errorf("Slice value in field {{ .Name }} was too long")
}

{{ if .PreserveNil }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that we now have this tag, do we need to duplicate the array code? We'll only have a compile error if the user intentionally specifies "preservenil" on an array, which, IMO, we should have anyways.

Copy link
Owner Author

Choose a reason for hiding this comment

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

hrm... my concern is that the template conditionals are getting overly complicated. We have the .IsArray check and the .PreserveNil check in the same template and i think theyre overlapping enough to make it really hard to read

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. This code is just getting a bit long. But you're right.

gen.go Show resolved Hide resolved
return xerrors.Errorf("Slice value in field {{ .Name }} was too long")
}

{{ if .PreserveNil }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. This code is just getting a bit long. But you're right.

gen.go Outdated
{{ end }}
{{ MajorType "cw" "cbg.MajByteString" (print "len(" .Name ")" ) }}

if _, err := cw.Write({{ .Name }}[:]); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to re-slice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And the "this re-slices" comment above is outdated.

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.

Missed a bug.

writefile.go Outdated Show resolved Hide resolved
Copy link

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Was going to suggest what @Stebalien said. This is technically more correct behaviour but will inevitably break downstreams if applied by default. Struct tag LGTM!

gen.go Show resolved Hide resolved
@Stebalien Stebalien merged commit 66e95c3 into master Jan 9, 2024
@Stebalien Stebalien deleted the feat/nil-slice-handling branch January 9, 2024 15:36
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.

3 participants