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.

Sign up for GitHub

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

Jump to bottom

Concrete structs #92

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Concrete structs #92

wants to merge 3 commits into from

Conversation

thomaspurchas
Copy link

@thomaspurchas thomaspurchas commented Jul 17, 2024 •
edited
Loading

This is half of the changes in #83 that only covers the changes to structs and pointer usages

All fields with structs are always pointers to structs, rather than concrete structs

When providing a API interface to a dataset (such as a Restful API), it's normal for optional fields to be pointers, and non-optional fields to concrete types, including structs. This approach minimises the number of nil checks people have to do, and generally Go discourages the usage of pointers unless they're actually needed because you want to share a value.

Making every struct value in the generated code is very unergonomic, as it either requires nil checks littered at every level of your code that interacts with these structs, or (more likely) people don't bother with the nil checks, relying on Pkl to enforce the existence of values. However that approach means there's no indication as to what might break if a non-optional field is later make optional. By following go conventions, this change will result in build failures, rather the current runtime failures.

@thomaspurchas
Change generated go code to follow go conventions
5e5666a 
Normal go conventions encourage the returning on structs rather than
pointers or interfaces. It's also normal convension to make optional
values pointers, and non-optional concrete types, including stucts,
rather than pointers to structs.

As it very easy for a someone to choose to take a pointer to a struct in
go and pass that around if they want to avoid copies (although the go
compiler is heavily optimised to support passing complete structs, even
large ones, around), there's little benifit in making embeded structs
pointers. It just requires that downstream code needs to be filled with
annoying nil checkes, and generally makes it harder to write robust,
clean code.
@thomaspurchas
Update the test fixtures using the new codegenerator
a99d45c 
Codegen changes means that the old test fixtures are out of date, and
can't be used to test the go binding code. Updating these fixtures
breaks the binding code, and identifies the areas that need enhancement.
@thomaspurchas
Add support for decoding into concrete structs
35c2730 
The codegen changes means we need to support decoding data into concrete
structs, and not just pointers to structs.
@thomaspurchas thomaspurchas marked this pull request as ready for review July 17, 2024 21:05
@thomaspurchas thomaspurchas mentioned this pull request Jul 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@thomaspurchas