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
Pull in set of reserved words from Redshift docs #117
Conversation
sqlalchemy_redshift/dialect.py
Outdated
@@ -96,6 +98,36 @@ class RedshiftImpl(postgresql.PostgresqlImpl): | |||
\s* \) \s* # Arbitrary whitespace and literal ')' | |||
""", re.VERBOSE) | |||
|
|||
# Reserved words as extracted from Redshift docs. | |||
# Command used to extract: | |||
# curl -q "http://docs.aws.amazon.com/redshift/latest/dg/r_pg_keywords.html" | tr '\n' '\r' | sed 's/.*\(AES128.*WITHOUT\).*/\1/' | tr '\r' '\n' | sed 's/^\([A-Z0-9_]*\).*/"\1",/' | paste -s -d' ' - | fold -s -w 70 | awk '{print " "$0}' | tr 'A-Z' 'a-z' # noqa |
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'd probably just create a reserved words file and read it with
RESERVED_WORDS = set(pkg_resources.resource_string(__name__, 'reserved-words.txt').split('\n'))
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 also port your shell script to python and move it somewhere outside of sqlalchemy_redshift?
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 postgres base dialect has the list hardcoded, so I like keeping with what that's doing. This seems like the simplest approach to make it obvious what's going on, rather than adding indirection by separating it to a different file.
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.
OK, I guess it depends how often it changes.
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.
put the script in './script.sh" and refer to it in the comment, rather than the whole thing with #noqa
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.
oh, and of course the URL is https://docs.aws.amazon.com/redshift/latest/dg/r_pg_keywords.html
sqlalchemy_redshift/dialect.py
Outdated
@@ -96,6 +98,36 @@ class RedshiftImpl(postgresql.PostgresqlImpl): | |||
\s* \) \s* # Arbitrary whitespace and literal ')' | |||
""", re.VERBOSE) | |||
|
|||
# Reserved words as extracted from Redshift docs. | |||
# Command used to extract: | |||
# curl -q "http://docs.aws.amazon.com/redshift/latest/dg/r_pg_keywords.html" | tr '\n' '\r' | sed 's/.*\(AES128.*WITHOUT\).*/\1/' | tr '\r' '\n' | sed 's/^\([A-Z0-9_]*\).*/"\1",/' | paste -s -d' ' - | fold -s -w 70 | awk '{print " "$0}' | tr 'A-Z' 'a-z' # noqa |
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.
put the script in './script.sh" and refer to it in the comment, rather than the whole thing with #noqa
Moved the script to pull_reserved_words.sh |
pull_reserved_words.sh
Outdated
@@ -0,0 +1,17 @@ | |||
#!/bin/sh |
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.
nit #!/usr/bin/env sh
sqlalchemy_redshift/dialect.py
Outdated
@@ -96,6 +98,36 @@ class RedshiftImpl(postgresql.PostgresqlImpl): | |||
\s* \) \s* # Arbitrary whitespace and literal ')' | |||
""", re.VERBOSE) | |||
|
|||
# Reserved words as extracted from Redshift docs. | |||
# See pull_reserved_words.sh in this package for the code used to |
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.
nit, technically it's not in the package, it's just outside the package ;)
nice |
I'm going to go ahead and try cutting a release. |
Addresses #94
Todos
[ ] Documentationnot needed for this change