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

Configuration option to allow override/hook to identifier wrapping code #2217

Merged
merged 1 commit into from
Sep 8, 2017

Conversation

elhigu
Copy link
Member

@elhigu elhigu commented Sep 7, 2017

First part of #2084 and #929 feature, which allows to write custom identifier wrapper methods.

So effectively you can describe function how each identifier name are treated, like for example id replacement in knex.raw('??', ['MyTable.myCol']) will behave. This is pretty nice feature for ORMs which would like to allow for example different naming scheme for database tables/columns and on javascript side.

Documentation PR is here knex/documentation#51

@stevenvachon
Copy link

stevenvachon commented Sep 7, 2017

By "first part", are you referring to the transformation of identifiers with no concept of direction? The code appears to always convert to snake_case, instead of conditionally to that or camelCase.

@elhigu
Copy link
Member Author

elhigu commented Sep 7, 2017

@stevenvachon by the "first part" I mean converting identifiers and with "second part" I mean converting column names of the result rows read from database. Did you check the documentation pull request, which explains what the method does? If you have suggestions how to improve that all the comments are appreciated.

Also there is no hard coded snake_case <-> camelCase conversion in this PR. Just a tool for making implementation for that easier. Converting column names of result rows read from database is completely different feature, but together those two features can be used to create the complete snake_case <-> camelCase conversion.

@stevenvachon
Copy link

stevenvachon commented Sep 8, 2017

The documentation is in a script file which doesn't make for the clearest to comprehension. As for the snake_case -> camelCase, I was referring to the test file (which instead concatenates).

@elhigu
Copy link
Member Author

elhigu commented Sep 8, 2017

As for the snake_case -> camelCase, I was referring to the test file (which instead concatenates).

Yes. That is very much intentional. It just tests that modifying and calling original wrapped callback actually works.

@elhigu
Copy link
Member Author

elhigu commented Sep 8, 2017

@wubzz @rhys-vdw @tgriesser @koistya any comments on this one?

@wubzz wubzz self-requested a review September 8, 2017 07:44
Copy link
Member

@wubzz wubzz left a comment

Choose a reason for hiding this comment

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

Nice simple solution to something that has been requested for a long time. I have no objections!

@elhigu
Copy link
Member Author

elhigu commented Sep 8, 2017

I'll still check if I'm able to add info, which part of the identifier is being transformed. Somewhat the same way that @koistya mentioned in issue #2084.

EDIT: I'm pretty sure that there is no robust way to this. For example with knex.raw('??', ['name']) there is no way to tell if the name is table name, column name, schema name or an alias...

@sdwrage
Copy link

sdwrage commented Sep 22, 2017

Is there an estimated timeline for when a pull request will be made to the main knex repo for the last half of this feature? In other words, when will the full implementation be available for use?

@elhigu
Copy link
Member Author

elhigu commented Sep 23, 2017

@sdwrage do you mean when this one is released or if there is going to be also hook for modifying column names read from database? Anyway, both features will be released in next release knex 0.14.0 hopefully next month. There are some other PRs still which I would like to get merged before it.

@sdwrage
Copy link

sdwrage commented Sep 25, 2017

@elhigu both and thank you for the answer :)

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.

4 participants