-
Notifications
You must be signed in to change notification settings - Fork 33
Make generated code better match Go conventions, and improve usage ergonomics #83
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
Open
thomaspurchas
wants to merge
4
commits into
apple:main
Choose a base branch
from
thomaspurchas:match-go-convensions
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5cf1012 to
a1d01f8
Compare
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. Finally rather than having interfaces and "Impl" structs, this change swaps those around. Structs don't have any postfix at all, and interfaces now have an I prefix. The big advantage of this approach is that it makes struct naming much more stable. Previously making a Pkl class open or abstract would result in the generated structs changing their names include "Impl", and the original names becoming interfaces. In turn breaking any code already using the existing structs. Now those breakages won't occur, and devs can opt into using the newly generated interfaces as needed, rather than having to re-write existing code to support the change.
We've made some big changes in how the codegen works, so we need to update all the snippet outputs to reflect that.
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.
The codegen changes means we need to support decoding data into concreet structs, and not just pointers to structs.
a1d01f8 to
603e96e
Compare
This was referenced Jul 1, 2024
Member
|
Per our conversation: please split this into two PRs. Thanks! |
Closed
Contributor
Author
Member
|
Sounds good! Will take a look. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current generated go code has a couple of quirks in it what makes it more difficult to work with than it needs to be. Two primary culprits are:
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 ofnilchecks 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
structvalue in the generated code is very unergonomic, as it either requiresnilchecks littered at every level of your code that interacts with thesestructs, or (more likely) people don't bother with thenilchecks, 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.Structs sometimes have an
Implpostfix and sometimes don'tCurrently the generated code will create an
Interfacefor everyopenorabstractPkl class, and name the associatedstructSomthingImpl. There's nothing inherently wrong with this approach, but it does make it unnecessarily painful to change a Pkl class from closed to open, or visa-versa.If a class is made
openthen the existing Gostructwill haveImplappended to its name, and its existing name will become aninterface, breaking all existing code that previously referenced the originalstruct. There's no need for this breakage as there is also a Go convention to prefix interfaces withI(although frowned upon, but no more than appendingImpltostructs). Using the interface prefix, rather than thestructpostfix, means that the names ofstructs remains static regardless of the modifiers on the Pkl class. Thus when a Pkl class is madeopena newISomethinginterface is created, but the existingSomthingstructremains untouched, preventing unnecessary breakage.Example of changes
These changes are probably best viewed by looking at the diffs of the snippet test outputs, but here's an example that demonstrates most the resulting changes to the generated go code.
Original:
New: