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 psycopg2.extensions.quote_ident. #359

Merged
merged 2 commits into from Oct 15, 2015

Conversation

Projects
None yet
2 participants
@a1exsh
Copy link
Contributor

a1exsh commented Oct 14, 2015

@dvarrazzo how about this way? #308

The compose stuff looks nice, but I only need something really simple and straightforward for #322: https://github.com/psycopg/psycopg2/pull/322/files#diff-e6256dfa32f9bced2faae91403f4d594R487

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Oct 14, 2015

I'm fine with it, I can merge it, but I would like to see unicode and Py3 tests.

What does the function return? str or unicode? what on py3? I think it should take both input, encode unicode in the connection encoding (use psycopg_ensure_bytes()) and I think it should always emit bytes, but py3 guys may not agree, they like useless encoding/decoding roundtrips, so probably conn_text_from_chars() is the right choice, yes, but I would like to see it checked in the test suite.

More notes on the way...

@dvarrazzo

This comment has been minimized.

Copy link

dvarrazzo commented on doc/src/extensions.rst in 9295bce Oct 14, 2015

Yes it does. What if missing? Please raise NotSupported in case.

@dvarrazzo

This comment has been minimized.

Copy link

dvarrazzo commented on psycopg/psycopgmodule.c in 9295bce Oct 14, 2015

This is not an help, just the signature

@dvarrazzo

This comment has been minimized.

Copy link

dvarrazzo commented on psycopg/psycopgmodule.c in 9295bce Oct 14, 2015

s doesn't work, it decodes in ascii, breaking unicode. Please use O and have psycopg_ensure_bytes() to check if that was ok. See my changes on a previous patch of yours at 5afeee3 for an example.

This comment has been minimized.

Copy link

dvarrazzo replied Oct 14, 2015

Please support keyword arguments too. See the above patch again for an example

@dvarrazzo

This comment has been minimized.

Copy link

dvarrazzo commented on tests/test_quote.py in 9295bce Oct 14, 2015

Please test unicode input: my snowman friend ☃ (u"\u2603") must pass through the function unscratched and unmelted, just a bit quoted :)

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Oct 14, 2015

I'm happy to merge this changeset to help you with your work on the streaming replication branch. Of course if we end up improving the quote_ident support as discussed in #308 we can refactor streaming replication to use the new method.

@a1exsh

This comment has been minimized.

Copy link
Contributor

a1exsh commented Oct 14, 2015

Thank you! Will update to address the comments.

@a1exsh

This comment has been minimized.

Copy link
Contributor

a1exsh commented Oct 15, 2015

@dvarrazzo dvarrazzo merged commit 89bb6b0 into psycopg:master Oct 15, 2015

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Oct 15, 2015

Thank you, that was exactly what I expected.

Good luck with the replication stuff :)

@a1exsh

This comment has been minimized.

Copy link
Contributor

a1exsh commented Oct 15, 2015

Cool, many thanks! :)

@a1exsh a1exsh deleted the zalando-stups:feature/extensions-quote-ident branch Oct 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment