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

Make sqlite3 hasColumn case insensitive #3435

Merged

Conversation

@alexandrebodin
Copy link
Contributor

alexandrebodin commented Sep 11, 2019

This PR makes the sqlite3 hasColumn function case insensitive to respect sqlite3 convention.

Fix #3434.

It might some new tests ? let me know and I'll add the necessary ones.

@ricardograca

This comment has been minimized.

Copy link
Member

ricardograca commented Sep 11, 2019

Yes, this needs tests.

@alexandrebodin

This comment has been minimized.

Copy link
Contributor Author

alexandrebodin commented Sep 12, 2019

Hi @ricardograca,

I have looked into the test/integration/schema to add some tests about the hasColumn. The first thing I noticed is that test set a custom wrapIdentifier that upercases the values hence make the code case-insensitive. But in reality the default implementation doesn't do that. I don't understand why use this custtom wrapIdentifier and If I should remove it.

I might need a few explanations before adding the tests :)

Thanks !

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 22, 2019

@alexandrebodin I think that the reason why these wrappers are used is to test whether wrapping/postprocessing logic works in the first place. If default implementation is updated to do the wrapping by default, existing wrappers would need to be replaced with something else (e. g. applying some kind of prefixes).

@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented Sep 22, 2019

There should be custom wrappers only in few tests, which are exactly making sure that overriding those wrappers works as expected. Default wrappers should be restored after those tests if those wrapper tests were not separate knex instances in the first place.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 6, 2019

@alexandrebodin Do you need any additional help?

@alexandrebodin

This comment has been minimized.

Copy link
Contributor Author

alexandrebodin commented Oct 7, 2019

@kibertoad I will be able to work on it next week ;)

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 28, 2019

@alexandrebodin Any updates :)?

@alexandrebodin

This comment has been minimized.

Copy link
Contributor Author

alexandrebodin commented Oct 28, 2019

@kibertoad I'm here ! :) Will do the update soon !

@alexandrebodin alexandrebodin force-pushed the alexandrebodin:fix/sqlite-has-column-case-sensitive branch from 87a92b2 to bba473e Nov 4, 2019
@alexandrebodin alexandrebodin force-pushed the alexandrebodin:fix/sqlite-has-column-case-sensitive branch from bba473e to f7c5fb1 Nov 4, 2019
return Promise.resolve();
}

it('checks whether a column exists without being case sensitive, resolving with a boolean', function() {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 10, 2019

Collaborator

Could you use arrow function instead of function() and async/await instead of then chain?

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Nov 12, 2019

Author Contributor

Sure. Not a problem even if not consistent with the rest of the file ?

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 12, 2019

Collaborator

yeah, we want to eventually convert everything

@kibertoad kibertoad merged commit 0214f11 into knex:master Nov 14, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexandrebodin alexandrebodin deleted the alexandrebodin:fix/sqlite-has-column-case-sensitive branch Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.