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

support multiple columns in `.orderBy()` #2881

Merged
merged 4 commits into from Nov 6, 2018

Conversation

Projects
None yet
3 participants
@HurSungYun
Copy link
Contributor

HurSungYun commented Oct 27, 2018

implementation of #2874

@HurSungYun HurSungYun changed the title WIP: support multiple columns in `.orderBy()` support multiple columns in `.orderBy()` Oct 27, 2018

value: columnInfo['column'],
direction: columnInfo['order'],
});
} else if (typeof columnInfo === 'string') {

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018

Collaborator

why not use _.isString?

This comment has been minimized.

@HurSungYun

HurSungYun Oct 29, 2018

Contributor

changed it. thanks

@@ -549,6 +552,28 @@ assign(Builder.prototype, {
return this;
},

// Adds a `order by` with multiple columns to the query.
orderByArray(columnDefs) {

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018

Collaborator

is this supposed to be a public method?

This comment has been minimized.

@HurSungYun

HurSungYun Oct 29, 2018

Contributor

I've changed it to private method

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 28, 2018

Thank you for your contribution! Would you consider creating documentation in https://github.com/knex/documentation for it?

@HurSungYun

This comment has been minimized.

Copy link
Contributor

HurSungYun commented Oct 29, 2018

@kibertoad Okay I will make a PR for this feature. thanks for reviewing :)

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 29, 2018

LGTM! @elhigu what do you think?

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 6, 2018

Everything seemed fine to me 👍

@kibertoad kibertoad merged commit 4db047d into tgriesser:master Nov 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 84.735%
Details

mwilliammyers added a commit to voxjar/knex that referenced this pull request Dec 12, 2018

support multiple columns in `.orderBy()` (tgriesser#2881)
* support multiple columns in `.orderBy()`

* add unit tests of order by

* orderByArray function to private

* use isString in string check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment