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

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

Merged
merged 4 commits into from Nov 6, 2018

Conversation

@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.

Copy link
@kibertoad

kibertoad Oct 28, 2018

Collaborator

why not use _.isString?

This comment has been minimized.

Copy link
@HurSungYun

HurSungYun Oct 29, 2018

Author 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.

Copy link
@kibertoad

kibertoad Oct 28, 2018

Collaborator

is this supposed to be a public method?

This comment has been minimized.

Copy link
@HurSungYun

HurSungYun Oct 29, 2018

Author 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 Author

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
Member

elhigu commented Nov 6, 2018

Everything seemed fine to me 👍

@kibertoad kibertoad merged commit 4db047d into knex:master Nov 6, 2018
2 checks passed
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()`

* 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.