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

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 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 commented Jan 22, 2017

and thanks for contribution 👍 !

@tomaspinho
Copy link
Contributor Author

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taken care of ;) thanks!

@elhigu elhigu merged commit 7de07c9 into knex:master Jan 26, 2017
@elhigu
Copy link
Member

elhigu commented Jan 26, 2017

Thanks @tomaspinho

@tomaspinho
Copy link
Contributor Author

Thank you, @elhigu 😄

elhigu pushed 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants