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

Use wrapIdentifier in columnInfo. fixes #2402 #2405

Conversation

@koskimas
Copy link
Collaborator

@koskimas koskimas commented Jan 2, 2018

Hi,

currently columnInfo doesn't respect the wrapIdentifier function if one is provided in the configuration. This PR fixes that.

fixes #2402

koskimas added 2 commits Jan 2, 2018
 * Also fixes a bug where only single digit indices were accepted.
 * customWrapIdentifier is one more level of abstraction for the wrapIdentifier "pipeline"
   that allows the custom wrapIdentifier to be called without calling the `wrapIdentifierImpl`
@elhigu
Copy link
Member

@elhigu elhigu commented Jan 2, 2018

Thanks a lot for PR! I'm fixing the test suite right now... check this one after tests are running again.

@koskimas koskimas force-pushed the koskimas:use-wrapIdentifier-and-postProcessResponse-hooks-in-columnInfo branch from 46cb537 to 820bc9f Jan 2, 2018
return this.customWrapIdentifier(value, this.wrapIdentifierImpl);
},

customWrapIdentifier(value, origImpl) {

This comment has been minimized.

@elhigu

elhigu Jan 2, 2018
Member

Kind of hacky, but works at least as long as user doesn not implement their custom quote style too in their wrapIdentifier configuration. Maybe knex would need this approach to be more official and to have separate APIs to override for quoting and for transforming identifiers. Anyways this is good enough for now.

This comment has been minimized.

@koskimas

koskimas Jan 2, 2018
Author Collaborator

Yeah, but there is no other way to do this right now.

@elhigu elhigu merged commit 45f5ffb into knex:master Jan 2, 2018
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
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.

2 participants