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

Add support for CODEC column option #89

Merged
merged 3 commits into from
Mar 9, 2020
Merged

Add support for CODEC column option #89

merged 3 commits into from
Mar 9, 2020

Conversation

athre0z
Copy link
Contributor

@athre0z athre0z commented Mar 9, 2020

Continuing the discussion from here, this implements the required changes to add support for the dialect-specific CODEC(...) column option. Thank you for the hints regarding the implementation, they proved helpful in getting this started!

I got a little bit confused by your original message mentioning visit_column and realizing that this method only generates the column name, however looking around in the MSSQL implementation revealed that overriding get_column_specification appears the be the way to go. I never thought that MSSQL would prove to be helpful for anything in my life, but I stand corrected.

In the edited comment you changed it to visit_create_column, which I tested and can confirm that having the code in this method would also work. This PR currently implements the get_column_specification approach, but I could just as well move the code down a few lines if you'd prefer that -- just say the word.

Also, would you want to advertise this feature in the README?

Fixes #88

@coveralls
Copy link

coveralls commented Mar 9, 2020

Coverage Status

Coverage increased (+0.09%) to 93.939% when pulling 433c300 on athre0z:feature/column-codec into d4be4a8 on xzkostyan:master.

@xzkostyan
Copy link
Owner

Excellent PR!

Yep, you can add an example of codecs usage in section SQLAlchemy declarative support.

@athre0z
Copy link
Contributor Author

athre0z commented Mar 9, 2020

I added a column with codec overrides to the existing README example and, since I assume that these aren't covered by CI testing, verified that it is still working by pasting it into a project of mine. Seems to be working fine!

fNmLEM8nHS3Ki2X8

(I renamed it to RateXX since Rate was already in use :D)

@xzkostyan xzkostyan merged commit 413d737 into xzkostyan:master Mar 9, 2020
@athre0z athre0z deleted the feature/column-codec branch March 9, 2020 11:51
@xzkostyan
Copy link
Owner

This requires SA>=1.3

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.

Support CODEC column expressions
3 participants