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

Projects
None yet
2 participants
@elhigu
Collaborator

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

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

This comment has been minimized.

Collaborator

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 tgriesser:master Sep 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

elhigu added a commit to ivanslf/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