Skip to content

feat: don't make dest argument variadic#109

Open
yansal wants to merge 1 commit intomasterfrom
feat/do-not-make-dest-argument-variadic
Open

feat: don't make dest argument variadic#109
yansal wants to merge 1 commit intomasterfrom
feat/do-not-make-dest-argument-variadic

Conversation

@yansal
Copy link
Copy Markdown
Contributor

@yansal yansal commented Feb 8, 2021

The API of the package is more clear if the dest argument of the Exec
function is not variadic. If users want to call the Exec function
without a dest argument, they can directly call driver.Exec instead.

@yansal yansal requested review from novln and thoas February 8, 2021 11:45
Copy link
Copy Markdown
Contributor

@novln novln left a comment

Choose a reason for hiding this comment

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

Destination (or output) is optional in this case, while I agree, most of the time we'll have something.

Using variadic for optional arguments is a known pattern in Go, while it's not the best, I think it's more simple/efficient to use (from an user perspective):

updateStmt := loukoum.Update("users").
    Set(loukoum.Pair("username", "novln")).
    Where(loukoum.Condition("username").Equal(1234))

makroud.Exec(ctx, driver, updateStmt)

Than,

makroud.Exec(ctx, driver, updateStmt, nil)

I'm not convinced by these changes. While I regret we can't have function overloading for this specific use case.

I don't have a better solution in my mind, but I think we could take advantage of this refactoring to merge RawExec and RawExecArgs, that basically does the same thing and has the same problems.

@yansal
Copy link
Copy Markdown
Contributor Author

yansal commented Feb 9, 2021

Using variadic for optional arguments is a known pattern in Go, while it's not the best, I think it's more simple/efficient to use (from an user perspective):

updateStmt := loukoum.Update("users").
Set(loukoum.Pair("username", "novln")).
Where(loukoum.Condition("username").Equal(1234))

makroud.Exec(ctx, driver, updateStmt)
Than,

makroud.Exec(ctx, driver, updateStmt, nil)

I would suggest to do the following instead:

query, args := loukoum.Update("users").
    Set(loukoum.Pair("username", "novln")).
    Where(loukoum.Condition("username").Equal(1234)).
    Query()
err := driver.Exec(ctx, query, args...)

I find it simpler because users just interacts with makroud.Driver, they don't need to interact with both makroud.Driver and makroud.Exec.

The API of the package is more clear if the dest argument of the Exec
function is not variadic. If users want to call the Exec function
without a dest argument, they can directly call driver.Exec instead.
@yansal yansal force-pushed the feat/do-not-make-dest-argument-variadic branch from 12edec7 to 4e0d300 Compare February 15, 2021 16:17
@yansal yansal changed the base branch from feat/remove-delete-and-archive to master February 15, 2021 16:17
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.

3 participants