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

Include namespace in bound keywords #125

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

Conversation

nivekuil
Copy link
Contributor

closes #118

Tested for a while now. I think this is the fastest way to strip the colon from a keyword.

@mpenet
Copy link
Owner

mpenet commented Sep 11, 2022

Can you make sure there is no reflection going on?

Adding a test would also be a good thing.

Thanks

@nivekuil
Copy link
Contributor Author

ah, there was reflection going on - thanks for catching that! For reference, measured with criterium it's 75ns with reflection, 4ns without. Compare to (subs (str foo) 1) which is 22ns.

I believe I found two bugs while writing the tests, for non-prepared statements in query->statement. First the namespace of the key should be kept as with this patch, and second it must also be double quoted to be used with a / in the name. I think the second is an existing bug, because it would prevent the use of reserved words as keys, e.g. {:values {"view" 1}} would have to be {:values {"\"view\"" 1}}. I added a naive format but this would also break existing users who already quoted these explicitly -- should we be smarter and check if it's already quoted? I added additional tests for reserved words in column names.

@mpenet
Copy link
Owner

mpenet commented Sep 12, 2022

I was thinking about this some more and I am a bit torn. Keywords translate to unquoted identifiers currently, maybe that should remain as is and push the conversion burden to the user, for simplicity's sake. I have to think about it a bit, I ll let it rest for now.

maybe @mccraigmccraig has an opinion on this

@nivekuil
Copy link
Contributor Author

I think @mccraigmccraig wouldn't be notified if the mention is edited in, fyi

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.

Include namespace in bound named parameters?
2 participants