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

Add overrideable identifier preparation methods #124

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ClaireNeveu
Copy link

I'm using sql-bricks in my library and I'd like to add support for automatically converting between snake_case and camelCase.

I've added two overrideable methods on sql: sql._prepareTableIdentifier and sql._prepareColumnIdentifier. By overriding these you can change the behavior of the auto-quoter or simply hook into that pipeline and then pass the identifier on. This is a global mutation which isn't ideal but it's the best we can do with the current state of sql-bricks.

I know there's rumblings about a rewrite that would make it easier to override via inheritance, but since that is a way off I think this would be an acceptable interim solution.

Matching issue: #101

}

function prepareTableIdentifier(table, opts) {
return prepareColumnIdentifier(expandAlias(table), opts);
Copy link
Author

Choose a reason for hiding this comment

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

This intentionally points to prepareColumnIdentifier rather than sql._prepareColumnIdentifier so that changing the column escaping behavior doesn't incidentally change the table escaping behavior.

@prust
Copy link
Collaborator

prust commented Sep 21, 2020

Thanks for contributing this, @ClaireNeveu! I'll have to make some time to look at it in more detail.

One thing I noticed is that it seems to allow SQL functions to be used directly, where column names are expected -- I think sql-bricks previously required them to be wrapped in sql('...'). Allowing them to be passed in directly like this, with no wrapping, would be nice.

The rewrite does seem to be a long way off, as I haven't had much need or motivation to work on it and my attention has been consumed with other projects. An interim solution like this does seem like a good idea.

@ClaireNeveu
Copy link
Author

ClaireNeveu commented Sep 21, 2020

Sql bricks currently allows you to do select('count(*)').from('my_table') without wrapping the functions and that is actually how the docs recommend you use functions. I just check if what's being passed is a function and exclude that from any potential overrides

In my library I actually add an extra layer requiring you to do select(fn.count('*')).from('my_table') because I differentiate between columns and functions at the type level, but I wasn't planning to push that upstream.

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.

2 participants