-
Notifications
You must be signed in to change notification settings - Fork 151
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
Conversation
MetaData(), | ||
Column('id', Integer, primary_key=True), | ||
Column('name', String), | ||
redshift_sortkey=unicode("id")) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
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
.
+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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
c30d2f6
to
f705281
Compare
@graingert how's this look? |
if py3k: | ||
string_types = str, | ||
else: | ||
string_types = basestring, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Looks good to me, have a look at the fixes then update the issue name, changelog, squash etc |
done |
Fix unicode issue with SORTKEY on python 2
Fix unicode issue with SORTKEY on python 2
This is to make sure unicode is supported in python2
@graingert