fix: fix return list type is not slice of pointer of struct #9575
8000Conversation
| func (funcs goTemplateFuncs) FormatReturnType(f introspection.Field, scopes ...string) (representation string, err error) { | ||
| scope := strings.Join(scopes, "") | ||
| ff := funcs.CommonFunctions.FormatTypeFuncs.WithScope(scope) | ||
|
|
||
| for ref := f.TypeRef; ref != nil; ref = ref.OfType { | ||
| switch ref.Kind { | ||
| case introspection.TypeKindNonNull: | ||
| continue | ||
| case introspection.TypeKindList: | ||
| defer func() { | ||
| representation = ff.FormatKindList(representation) | ||
| }() | ||
| case introspection.TypeKindObject: | ||
| representation = ff.FormatKindObject(representation, ref.Name, funcs.CommonFunctions.ConvertID(f)) | ||
| representation = "*" + representation | ||
| return | ||
| default: | ||
| return funcs.CommonFunctions.FormatType(ref, scope, funcs.CommonFunctions.ConvertID(f)) | ||
| } | ||
| } | ||
| return "", fmt.Errorf("unexpected type kind %s", f.TypeRef.Kind) | ||
| } |
There was a problem hiding this comment.
This is quite a bit hack. Feel free to suggest if anyone has a better solution. 🙏
There was a problem hiding this comment.
I think it's fine that way.
A big refactor on how the codegen works will happen at some point and that will be a good time to fix all these small magic tricks.
d1ed977 to
ed4f395
Compare
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
| func (funcs goTemplateFuncs) FormatReturnType(f introspection.Field, scopes ...string) (representation string, err error) { | ||
| scope := strings.Join(scopes, "") | ||
| ff := funcs.CommonFunctions.FormatTypeFuncs.WithScope(scope) | ||
|
|
||
| for ref := f.TypeRef; ref != nil; ref = ref.OfType { | ||
| switch ref.Kind { | ||
| case introspection.TypeKindNonNull: | ||
| continue | ||
| case introspection.TypeKindList: | ||
| defer func() { | ||
| representation = ff.FormatKindList(representation) | ||
| }() | ||
| case introspection.TypeKindObject: | ||
| representation = ff.FormatKindObject(representation, ref.Name, funcs.CommonFunctions.ConvertID(f)) | ||
| representation = "*" + representation | ||
| return | ||
| default: | ||
| return funcs.CommonFunctions.FormatType(ref, scope, funcs.CommonFunctions.ConvertID(f)) | ||
| } | ||
| } | ||
| return "", fmt.Errorf("unexpected type kind %s", f.TypeRef.Kind) | ||
| } |
There was a problem hiding this comment.
I think it's fine that way.
A big refactor on how the codegen works will happen at some point and that will be a good time to fix all these small magic tricks.
|
Ideally we should version-compat this change - it has the potential to break published modules, so should have a check here. |
I’ve never dealing with a version compat before. How to do it? |
|
@wingyplus see https://github.com/dagger/dagger/pull/9118/files#diff-8062ad596c2b352a00e64f30eb9fff7ade878660570bf2c3c5f5aa727dd10496R864 for an example of where I've done this recently. Generally:
|
|
@wingyplus Any news on this PR? I'm ready to approve whenever you address changes :D |
May I know what scenario I should check here? My understanding is that the codegen binary is built into the dagger engine so it should fallback to previous behavior if we switch the binary back to previous version. Also, this PR breaks |
Changelog added. I'm waiting for CI to be complete. |
6ff2377 to
92e9718
Compare
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
@wingyplus It seems there's not conflict, please regenerate the dagger.gen.go client and then I can review :D |
Will fix in around 2-3 hours |
6c8668a to
8bd9407
Compare
|
@TomChv sorry for the delay. I rebased and squashed all the commits. 🙏 |
There was a problem hiding this comment.
Thanks for tackling the fix for this, I hit it yesterday and was happily surprised to see it was already being worked on. The changes here lgtm.
I agree w/ @jedevc that trying to make this backwards-compatible is important though. The reason being that there's plenty of user module code that will start failing to compile after this is fixed, which is extra bad when it's some external dependency module you as a user don't directly control and thus can't fix yourself.
As painful as it would be, I'd ideally want us to maintain the old behavior for modules w/ older engine versions in their dagger.json. I don't recall if we currently pass that dagger.json engine version information to codegen anywhere, but if we don't I think we could plumb it through to the codegen config (i.e. here) and use that to only have this new behavior for engine versions above the next one.
The code to support both is going to be ugly, but can be cleaned up after there's been enough time for the ecosystem to upgrade. This seems worth it because this fix is for something extremely important that we don't want to be stuck w/ forever.
Fixes dagger#9556 Fix any methods that return list of object must return in form of `[]*T`. The go template override `FormatReturnType` to make it works and expose `FormatType` from common functions type make it accessible from `FormatReturnType`. Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
@sipsma Thank you for the feedback. Let me take a look at the details and will return in around 1-2 days. 🙏 |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for 7 days with no activity. |
Fixes #9556
Fix any methods that return list of object must return in form of
[]*T.The go template override
FormatReturnTypeto make it works and exposeFormatTypefrom common functions type make it accessible fromFormatReturnType.