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

Fix type for AnalyticFunction #5304

Merged
merged 1 commit into from Aug 28, 2022
Merged

Fix type for AnalyticFunction #5304

merged 1 commit into from Aug 28, 2022

Conversation

patrick330602
Copy link
Contributor

Fix AnalyticFunction type affecting rowNumber, rank and DenseRank.

The problem

I discovered during development, where TypeScript complains:

Argument of type '{ column: string; order: "desc"; }' is not assignable to parameter of type '"oid" | "oid"[] | { columnName: "oid"; order?: "asc" | "desc" | undefined; nulls?: "first" | "last" | undefined; }'.
  Object literal may only specify known properties, and 'column' does not exist in type '"oid"[] | { columnName: "oid"; order?: "asc" | "desc" | undefined; nulls?: "first" | "last" | undefined; }'.

However, they are clearly using column in the code:

partitionBy(column, direction) {
assert(
Array.isArray(column) || typeof column === 'string',
`The argument to an analytic partitionBy function must be either a string
or an array of string.`
);
if (Array.isArray(column)) {
this.partitions = this.partitions.concat(column);
} else {
this.partitions.push({ column: column, order: direction });
}
return this;
}
orderBy(column, direction) {
assert(
Array.isArray(column) || typeof column === 'string',
`The argument to an analytic orderBy function must be either a string
or an array of string.`
);
if (Array.isArray(column)) {
this.order = this.order.concat(column);
} else {
this.order.push({ column: column, order: direction });
}
return this;
}

Also, this can verify that using columnName is apparently incorrect:

Screenshot 2022-08-18 at 15 34 57

affecting rowNumber, rank and DenseRank
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.27% when pulling 7cc863b on patrick330602:master into 2dadde4 on knex:master.

@OlivierCavadenti OlivierCavadenti merged commit 579ab4b into knex:master Aug 28, 2022
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.

None yet

3 participants