alia icon indicating copy to clipboard operation
alia copied to clipboard

Include namespace in bound keywords

Open nivekuil opened this issue 3 years ago • 4 comments

closes https://github.com/mpenet/alia/issues/118

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

nivekuil avatar Sep 11 '22 00:09 nivekuil

Can you make sure there is no reflection going on?

Adding a test would also be a good thing.

Thanks

mpenet avatar Sep 11 '22 11:09 mpenet

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.

nivekuil avatar Sep 11 '22 18:09 nivekuil

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

mpenet avatar Sep 12 '22 08:09 mpenet

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

nivekuil avatar Sep 18 '22 06:09 nivekuil