-
Notifications
You must be signed in to change notification settings - Fork 422
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
chore: Add ShowByID filtering generation #3227
base: main
Are you sure you want to change the base?
Changes from all commits
f4d01dd
a7366ec
bc0fb66
5cb03d1
8f2a301
1599afb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
package generator | ||
|
||
type ShowByIDFiltering struct { | ||
Kind string | ||
Args string | ||
IdentifierBased bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we could use an enum instead? What if we have more showByIDs which have a different behavior? Such flag could be confusing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, that would probably be a better approach than just a flag |
||
} | ||
|
||
type ShowByIDFilteringKind uint | ||
|
||
const ( | ||
// Enables filtering with: Like | ||
ShowByIDLikeFiltering ShowByIDFilteringKind = iota | ||
// Enables filtering with: In | ||
// Based on the identifier Kind | ||
ShowByIDInFiltering | ||
// Enables filtering with: ExtendedIn | ||
// Based on the identifier Kind | ||
ShowByIDExtendedInFiltering | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In your doc you proposed sth like
I'm not saying it's bad, but I'm curious about the other approach here. I wonder if we can separate these abstractions (one is database vs schema, and the other is in vs extended in) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we can skip the In vs. ExtendedIn because those filterings use different structs. For ExtendedIn, we need to provide the ExtendedIn struct, and for Simple In, just the In struct. I aimed to make it similar in use to what we have for the g.NewQueryStruct("ShowSecret").
...
OptionalLike().
OptionalExtendedIn(), and then for the filtering we use: ).ShowByIdOperation(
g.ShowByIDLikeFiltering,
g.ShowByIDExtendedInFiltering,
) |
||
) | ||
|
||
func (s *Operation) withFiltering(filtering ...ShowByIDFilteringKind) *Operation { | ||
for _, f := range filtering { | ||
switch f { | ||
case ShowByIDLikeFiltering: | ||
s.ShowByIDFiltering = append(s.ShowByIDFiltering, ShowByIDFiltering{ | ||
Kind: "Like", | ||
Args: "Pattern: String(id.Name())", | ||
IdentifierBased: false, | ||
}) | ||
case ShowByIDInFiltering: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use it (non-extended In) in one object to show if it works. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea, i will generate some objects to show all the filtering options |
||
s.ShowByIDFiltering = append(s.ShowByIDFiltering, ShowByIDFiltering{ | ||
Kind: "In", | ||
Args: "%[1]v: id.%[1]vId()", | ||
IdentifierBased: true, | ||
}) | ||
case ShowByIDExtendedInFiltering: | ||
s.ShowByIDFiltering = append(s.ShowByIDFiltering, ShowByIDFiltering{ | ||
Kind: "ExtendedIn", | ||
Args: "In: In{%[1]v: id.%[1]vId()}", | ||
IdentifierBased: true, | ||
}) | ||
} | ||
} | ||
return s | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
{{- /*gotype: github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk/poc/generator.Interface*/ -}} | ||
|
||
{{ $impl := .NameLowerCased }} | ||
|
||
{{ range .Operations }} | ||
{{ if and (eq .Name "Show") .ShowMapping }} | ||
func (v *{{ $impl }}) Show(ctx context.Context, request *{{ .OptsField.DtoDecl }}) ([]{{ .ShowMapping.To.Name }}, error) { | ||
|
@@ -13,9 +14,21 @@ | |
return resultList, nil | ||
} | ||
{{ else if eq .Name "ShowByID" }} | ||
{{ $idkind := .ObjectInterface.ObjectIdentifierKind }} | ||
func (v *{{ $impl }}) ShowByID(ctx context.Context, id {{ .ObjectInterface.IdentifierKind }}) (*{{ .ObjectInterface.NameSingular }}, error) { | ||
// TODO: adjust request if e.g. LIKE is supported for the resource | ||
{{ $impl }}, err := v.Show(ctx, NewShow{{ .ObjectInterface.NameSingular }}Request()) | ||
request := NewShow{{ .ObjectInterface.NameSingular }}Request() | ||
{{- if not .ShowByIDFiltering }} | ||
// TODO: adjust request if e.g. LIKE is supported for the resource | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can leave the default behavior as it is, but we can point the SDK users to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about: // TODO: adjust request if e.g. LIKE is supported for the resource with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. |
||
{{ else }} | ||
{{- range .ShowByIDFiltering -}} | ||
.{{- if .IdentifierBased }} | ||
With{{ .Kind }}( {{ .Kind }} { {{ printf .Args $idkind }} }) | ||
{{- else }} | ||
With{{ .Kind }}( {{ .Kind }} { {{ .Args }} }) | ||
{{- end }} | ||
{{- end }} | ||
{{- end }} | ||
{{ $impl }}, err := v.Show(ctx, request) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open for discussions, but I'm not sure if it's necessary. On the other hand, in Snowflake, is just
IN
, and initiallyExtendedIn
was used only to distinguish fromIn
with limited filtering capabilities.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will change how the filtering in the template is handled and how we should define filtering for the
ShowBiIDFiltering
struct. But it can be changed if it is preferred to leave theExtended In
as before.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, it's more readable that way because I know it's the extended version, but both options are ok.