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

Updated oprator list and allow operator transformation for e.g. escaping ? #2239

Merged
merged 1 commit into from Sep 23, 2017

Conversation

@elhigu
Copy link
Member

@elhigu elhigu commented Sep 22, 2017

Actually this is slightly breaking change, since now original format how operator was written is not preserved anymore. for example iLiKe transforms to ilike.

mainly added support for .where('col', '?', something)

@elhigu elhigu requested review from tgriesser, rhys-vdw and wubzz Sep 22, 2017
@wubzz
wubzz approved these changes Sep 22, 2017
Copy link
Member

@wubzz wubzz left a comment

I think this is a fine change, especially for querying json columns. As for the "enforced lowercase" of operator I don't really see it as an issue aside from that it may break some unit tests here and there.

To avoid breaking unit tests (such as the one changed in the PR) a small adjustment should take care of it:

const valueLower = _.toLower(value);
const operator = operators[valueLower];

if(!Operator) {...}

return valueLower === operator ? value : operator;

But that's up to you, I have no strong opinion.

@elhigu
Copy link
Member Author

@elhigu elhigu commented Sep 23, 2017

@wubzz since next release is already breaking one, I consider it ok to make normalization of operator at the same time. I think it was originally an accident to allow preserving case how operator was written in this case.

@elhigu elhigu merged commit 491cc78 into knex:master Sep 23, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
elhigu added a commit to ivanfilhoz/knex that referenced this pull request Oct 16, 2017
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

2 participants