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

ONLY keyword support for PostgreSQL #1874

Merged
merged 4 commits into from Jan 26, 2017
Merged

ONLY keyword support for PostgreSQL #1874

merged 4 commits into from Jan 26, 2017

Conversation

@tomaspinho
Copy link
Contributor

@tomaspinho tomaspinho commented Jan 20, 2017

Hello, we're requiring the ability to use the ONLY keyword in PostgreSQL and I have done a crude implementation of it in this PR.
Please discuss and provide any comments that would enable this or other PR to be merged that implements the same functionality.
Thank you!

… from() and into() through table(tableName, only)
@elhigu
Copy link
Member

@elhigu elhigu commented Jan 22, 2017

Sounds like ONLY is actually not postgresql specific functionality, but comes from standard... any ideas if mysql or others support it?

Generally pull request looks good, change is so simple that I'm fine that there is no integration tests for this since the change is so simple.

One thing there is that I would like to change. Instead of true/false parameter would be more future proof to pass { only: true } object instead.

Could you make also pull request to documentation repository and add link to it here, so I can check that documentation will be

@elhigu
Copy link
Member

@elhigu elhigu commented Jan 22, 2017

and thanks for contribution 👍 !

@tomaspinho
Copy link
Contributor Author

@tomaspinho tomaspinho commented Jan 22, 2017

Hey @elhigu! Thanks so much for the feedback. From Google searches, I would say Postgres is the only one supporting it, as other engines don't seem to support table inheritance in the same way (but I may be wrong, not a DBA).

I'll see if I can take a look at your request this coming week and update this PR accordingly.

@tomaspinho
Copy link
Contributor Author

@tomaspinho tomaspinho commented Jan 23, 2017

Hey @elhigu,

You may find the new docs here: knex/documentation#16
I've already implemented the options object, but the build seems to have failed in an integration test unrelated to this PR. Could you please make it run again? Thanks!

this._single.table = tableName;
this._single.only = options.only !== undefined ? options.only : false;

This comment has been minimized.

@wubzz

wubzz Jan 24, 2017
Member

Try to prevent negated conditions where possible. Here it would make more sense with explicit checks this._single.only = options.only === true as it would also ensure the property only is always a boolean. :)

This comment has been minimized.

@tomaspinho

tomaspinho Jan 25, 2017
Author Contributor

taken care of ;) thanks!

@elhigu elhigu merged commit 7de07c9 into knex:master Jan 26, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu
Copy link
Member

@elhigu elhigu commented Jan 26, 2017

Thanks @tomaspinho

@tomaspinho
Copy link
Contributor Author

@tomaspinho tomaspinho commented Jan 27, 2017

Thank you, @elhigu 😄

elhigu added a commit to elhigu/knex that referenced this pull request Feb 15, 2017
* adding support for ONLY keyword for PostgreSQL as an optional flag of from() and into() through table(tableName, only)

* passing only parameter from default function was missing

* changing only option to options object with only key

* better style per @wubzz suggestion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants