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

Fix unicode issue with SORTKEY on python 2 #34

Merged
merged 1 commit into from Sep 1, 2015

Conversation

bouk
Copy link
Contributor

@bouk bouk commented Sep 1, 2015

This is to make sure unicode is supported in python2

@graingert

MetaData(),
Column('id', Integer, primary_key=True),
Column('name', String),
redshift_sortkey=unicode("id"))
Copy link
Member

Choose a reason for hiding this comment

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

You can use u'id' and this will work on both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

The lint test runs on python 3 and throws an error for this unicode.

@jklukas
Copy link
Member

jklukas commented Sep 1, 2015

+1

@@ -11,6 +11,7 @@
from sqlalchemy.ext.compiler import compiles
from sqlalchemy.sql.expression import Executable, ClauseElement
from sqlalchemy.types import VARCHAR, NullType
from sqlalchemy.util.compat import string_types
Copy link
Member

Choose a reason for hiding this comment

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

When was this added to sqlalchemy? Is it considered public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm looking at it it's definitely not public. What would be the best way to go about this? If I were to copy over the code, the linter would complain about unicode or basestring

Copy link
Member

Choose a reason for hiding this comment

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

What about a compat.py with a # noqa line?

@bouk bouk force-pushed the check-unicode branch 3 times, most recently from c30d2f6 to f705281 Compare September 1, 2015 20:50
@bouk
Copy link
Contributor Author

bouk commented Sep 1, 2015

@graingert how's this look?

if py3k:
string_types = str,
else:
string_types = basestring,
Copy link
Member

Choose a reason for hiding this comment

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

I prefer parenthesis with my tuples

Copy link
Member

Choose a reason for hiding this comment

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

can you move the noqa to this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me too, I just copied the sa style. fixed

@graingert
Copy link
Member

Looks good to me, have a look at the fixes then update the issue name, changelog, squash etc

@bouk bouk changed the title Use sqlalchemy.util.string_types to check if it's a string Use a compatibility layer to check string types Sep 1, 2015
@bouk bouk changed the title Use a compatibility layer to check string types Fix unicode with SORTKEY on python 2 Sep 1, 2015
@bouk bouk changed the title Fix unicode with SORTKEY on python 2 Fix unicode issue with SORTKEY on python 2 Sep 1, 2015
@bouk
Copy link
Contributor Author

bouk commented Sep 1, 2015

done

graingert added a commit that referenced this pull request Sep 1, 2015
Fix unicode issue with SORTKEY on python 2
@graingert graingert merged commit be53c70 into sqlalchemy-redshift:master Sep 1, 2015
@bouk bouk deleted the check-unicode branch September 1, 2015 21:08
haleemur pushed a commit to haleemur/redshift_sqlalchemy that referenced this pull request Sep 2, 2015
Fix unicode issue with SORTKEY on python 2
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.

None yet

3 participants