Skip to content

Conversation

kyleconroy
Copy link
Collaborator

Fixes #761

@kyleconroy kyleconroy merged commit ba125cc into master Nov 19, 2020
@kyleconroy kyleconroy deleted the kyle/variadic-funcs branch November 19, 2020 16:55
@@ -251,6 +251,7 @@ type Argument struct {
Name string
Type *ast.TypeName
HasDefault bool
Variadic bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add this struct key when there is a mode for Variadic that you can use to check?

Name: "json_build_object",
Args: []*catalog.Argument{
{
Type: &ast.TypeName{Name: "any"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you mark Variadic arguments as having defaults because there can b 0 of them otherwise the following would fail.

postgres=# select json_build_object();
 json_build_object
-------------------
 {}
(1 row)


const selectJSONBuildObject = `-- name: SelectJSONBuildObject :one
SELECT
json_build_object('foo'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add the following edge case to the tests this will likely fail.

json_build_object(),

Also I would remove the following tests because they actually fail in postgresql with the error
"ERROR: argument list must have even number of elements"

  json_build_object('foo'),
  json_build_object('foo', 1, 2),

But catching those cases would be a little more difficult to catch without custom coding.

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.

Support for json_build_object is broken in 1.5
2 participants