Skip to content
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

feat(builder): Describe optional columnList arg to with/Recurisve [knex/knex#4514] #335

Merged

Conversation

jeremy-w
Copy link
Contributor

@jeremy-w jeremy-w commented Sep 3, 2021

These are the documentation updates to accompany knex/knex#4652, to resolve knex/knex#4514.

I'm not 100% I got the notation right; I'm not clear on whether it's [whatever] to signal an array vs [whatever] to signal an optional argument. But I hope the examples make the meaning clear.

@jeremy-w jeremy-w changed the title feat(querybuilder): Describe optional columnList arg to with/Recurisve [knex/knex#4514] feat(builder): Describe optional columnList arg to with/Recurisve [knex/knex#4514] Sep 3, 2021
@kibertoad
Copy link
Collaborator

@jeremy-w We've used [] for optional parameters, and for arrays we used plural form in the name of a parameter.

@@ -283,15 +283,21 @@ export default [
{
type: "method",
method: "with",
example: ".with(alias, function|raw)",
description: "Add a \"with\" clause to the query. \"With\" clauses are supported by PostgreSQL, Oracle, SQLite3 and MSSQL.",
example: ".with(alias, [*columns], callback|builder|raw)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

just columns should be fine, if it is a mandatory array param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both with(alias, query) and with(alias, columns, query) are valid. It sounds like [columns] would be the way to write it though, without the star?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yup, fair

@@ -283,15 +283,21 @@ export default [
{
type: "method",
method: "with",
example: ".with(alias, function|raw)",
description: "Add a \"with\" clause to the query. \"With\" clauses are supported by PostgreSQL, Oracle, SQLite3 and MSSQL.",
example: ".with(alias, [*columns], callback|builder|raw)",
Copy link
Collaborator

@kibertoad kibertoad Sep 3, 2021

Choose a reason for hiding this comment

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

btw, wouldn't it be more correct to say that third parameter is optional, and second one could be either columns, or whatever third parameter could be as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it’s equally correct, but I worry it obscures the intended behavior due to the complexity of the type signature:

with(alias, columns | (qb|callback|raw), (qb|callback|raw) | undefined)

The simplest way to document this IMO would be as two functions with different arities - which is how the TypeScript types are written, in fact. I don’t know that the documentation generator supports that notion of overloaded functions directly, though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I doubt it, so your original format might be better

@jeremy-w jeremy-w force-pushed the jeremy-w/feat/with-columnlist-4514 branch from 72fc95f to d7cd63a Compare September 4, 2021 17:23
@jeremy-w
Copy link
Contributor Author

jeremy-w commented Sep 4, 2021

Updated to use the [columns] styling of the optional column list.

@kibertoad kibertoad merged commit 3afc815 into knex:gh-pages Sep 4, 2021
@jeremy-w jeremy-w deleted the jeremy-w/feat/with-columnlist-4514 branch September 4, 2021 20:12
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.

Expand withRecursive database support via supporting column name declarations
2 participants