Skip to content

Use a callable to determine persisted enum values#410

Closed
razor-1 wants to merge 4 commits intozzzeek:masterfrom
razor-1:custom-enum-values
Closed

Use a callable to determine persisted enum values#410
razor-1 wants to merge 4 commits intozzzeek:masterfrom
razor-1:custom-enum-values

Conversation

@razor-1
Copy link
Copy Markdown
Contributor

@razor-1 razor-1 commented Dec 29, 2017

This intends to resolve https://bitbucket.org/zzzeek/sqlalchemy/issues/3906

The idea is to use values_callable as a new kwarg to Enum. It returns the values to be used, allowing for a variety of uses, including the one mentioned in the above issue.

Tests have been added as well as updates to the documentation. This is my first PR to sqlalchemy so apologies in advance for things that are out of place. Particularly I wonder if this needs to be supported in postgres as the work and tests I did were for mysql.

Comment thread test/dialect/mysql/test_types.py Outdated
import datetime
import decimal
from sqlalchemy import types as sqltypes
from enum import Enum as PyEnum
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

we don't need to pull this in, see test/sql/test_types.py line 1160 where build a minimal pep435 enumerated class.

Comment thread tox.ini Outdated
postgresql: psycopg2>=2.7
mysql: mysqlclient
mysql: pymysql
py27-mysql: enum34
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

not needed

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Dec 29, 2017

great! so first thing, this is the core Enum type so this feature needs tests at the base level in test/sql/test_types.py -> EnumTest. When you add there, you'll find the test likely fails for Postgresql since the same change we made to the MySQL version with adding the parameter to adapt_emulated_to_native also needs to happen in sqlalchemy/dialects/postgresql/base.py -> ENUM.

add values_callable in postgres adapt_emulated_to_native
add tests to test/sql/test_types
@razor-1
Copy link
Copy Markdown
Contributor Author

razor-1 commented Dec 31, 2017

Thanks for the feedback. I have attempted to address it in the latest commit.

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 6, 2018

havent forgotten you, just a lot on my list and also going out of town next week. this is on my short list.

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 17, 2018

great, let's test it

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 17, 2018

Dear contributor -

This pull request is being moved to Gerrit, at https://gerrit.sqlalchemy.org/#/c/zzzeek/sqlalchemy/+/636, where it may be tested and reviewed more closely. As such, the pull request itself is being marked "closed" or "declined", however your contribution is merely being moved to our central review system. Please register at https://gerrit.sqlalchemy.org#/register/ to send and receive comments regarding this item.

@zzzeek zzzeek closed this Jan 17, 2018
@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 17, 2018

it seems to have had some failures:

https://jenkins.sqlalchemy.org/job/sqlalchemy_gerrit/cext=cext,db=sqlite-postgresql,pyv=py27/1310/

test.dialect.mysql.test_types.EnumTest_postgresql+psycopg2_9_4_14.test_pep435_enum_values_callable_round_trip
test.dialect.mysql.test_types.EnumTest_sqlite+pysqlite_3_20_1.test_pep435_enum_values_callable_round_trip
test.dialect.mysql.test_types.EnumTest_postgresql+psycopg2_9_6_5.test_pep435_enum_values_callable_round_trip
test.dialect.mysql.test_types.EnumTest_postgresql+psycopg2_10_0.test_pep435_enum_values_callable_round_trip
test.sql.test_types.EnumTest_postgresql+psycopg2_10_0.test_pep435_enum_values_callable_round_trip
test.sql.test_types.EnumTest_postgresql+psycopg2_9_4_14.test_pep435_enum_values_callable_round_trip
test.sql.test_types.EnumTest_sqlite+pysqlite_3_20_1.test_pep435_enum_values_callable_round_trip
test.sql.test_types.EnumTest_postgresql+psycopg2_9_6_5.test_pep435_enum_values_callable_round_trip

if you have time, at least figuring out the sqlite failures would be a start, since that DB is embedded in Python already.

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 17, 2018

oh also those mysql/test_types/EnumTest should not have run for Postgresql and SQLite. that seems to be due to "from ...sql.test_types import EnumTest" in the MySQL test. I'd not include that in there. just re-state the fixture you need in your mysql-specific test suite.

@razor-1
Copy link
Copy Markdown
Contributor Author

razor-1 commented Jan 19, 2018

I think I have a fix for these things - at least for sqlite it's in better shape. I seem to get access denied errors trying to push to gerrit; is there a way to take the latest commit and add it as a new patch set?

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 19, 2018

if you make a username at https://gerrit.sqlalchemy.org/ I can give you access.

@razor-1
Copy link
Copy Markdown
Contributor Author

razor-1 commented Jan 19, 2018

I'm using the same username as on github, razor-1. thank you

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 19, 2018

can you add your email address to it which matches your github commit?

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 19, 2018

heh. did that crash my gerrit :) gerrit is very bad from an admin perspective

@razor-1
Copy link
Copy Markdown
Contributor Author

razor-1 commented Jan 19, 2018

I tried to add my email, but it doesn't seem possible at least with the oauth'd github login.

screen shot 2018-01-19 at 12 19 54 pm

I can create a different separate account if that would be better

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 19, 2018

ugh, the dropdown is empty ?

@razor-1
Copy link
Copy Markdown
Contributor Author

razor-1 commented Jan 19, 2018

There is no dropdown - none of the above things are clickable or take any input. Reload just reloads the page and displays the same thing.

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 19, 2018

try now ?

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 19, 2018

you're in

@razor-1
Copy link
Copy Markdown
Contributor Author

razor-1 commented Jan 19, 2018

yes, i've added an email address now, thanks

@zzzeek
Copy link
Copy Markdown
Owner

zzzeek commented Jan 19, 2018

so you're in the contributors group access should work..

@razor-1
Copy link
Copy Markdown
Contributor Author

razor-1 commented Jan 20, 2018

down to just postgres failing now. have reproduced it locally & working on a fix

@razor-1
Copy link
Copy Markdown
Contributor Author

razor-1 commented Jan 20, 2018

ok, fixed oracle. the mssql failures do not seem to be related to my changes, as far as I can tell. I also don't have easy access to mssql to run the tests. So I could use some help on that. Thank you

zzzeek pushed a commit that referenced this pull request Feb 8, 2018
Added support for :class:`.Enum` to persist the values of the enumeration,
rather than the keys, when using a Python pep-435 style enumerated object.
The user supplies a callable function that will return the string values to
be persisted.  This allows enumerations against non-string values to be
value-persistable as well.  Pull request courtesy Jon Snyder.

Pull-request: #410
Fixes: #3906
Change-Id: Id385465d215d1e5baaad68368b168afdd846b82c
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.

2 participants