djangonauts / django-hstore

PostgreSQL HStore support for Django.
http://django-hstore.readthedocs.io/
Other
517 stars 142 forks source link

compatibility with SQLAlchemy #50

Closed nemesifier closed 10 years ago

nemesifier commented 10 years ago

Aims of this pull request is to start working on:

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 1a5fb964addf13981363ae70501252c45ad36ca2 on features/sqlalchemy into a93ab32dde704970af5ac5563c716da267899d04 on master.

nemesifier commented 10 years ago

here's what I've found + some questions, please send feedback.

  1. I think HSTORE_GLOBAL_REGISTER should be passed to the deferred registration case too, as done in: 1a5fb96
  2. I introduced a new (temporary) setting called HSTORE_REGISTER_GLOBALLY which enables to pass globally=False if needed
  3. HSTORE_GLOBAL_REGISTER and HSTORE_REGISTER_GLOBALLY have similar names though, which is really a bad idea
  4. what's the consequence of passing unique=False?
  5. what's the consequence of passing globally=False?
  6. in my test environment, setting unique=True and globally=False breaks 20 tests (10 errors and 10 failures), while turning both globally and unique to False works (all tests pass)
  7. if my test environment would be the most common case it means there's no need for 2 separate settings and we can just use one, which I suggest being HSTORE_REGISTER_GLOBALLY by keeping backward compatibility with HSTORE_GLOBAL_REGISTER
coveralls commented 10 years ago

Coverage Status

Coverage increased (+0.01%) when pulling 5f07a2510b63190b977085a8c0528105805ff311 on features/sqlalchemy into a93ab32dde704970af5ac5563c716da267899d04 on master.

niwinz commented 10 years ago
niwinz commented 10 years ago
niwinz commented 10 years ago

The sqlalchemy compatible settings is: unique=False and globally=False

nemesifier commented 10 years ago

On 06/23/2014 04:46 PM, Andrey Antukh wrote:

The sqlalchemy compatible settings is: |unique=False| and |globally=False|

  • (7) You are proposing |HSTORE_REGISTER_GLOBALLY| replace the |HSTORE_GLOBAL_REGISTER| (obviously with backward compatibilities...)?

I ask the question in another way, does it have sense to have 2 separate settings to manage both parameters or should we only have one?

If we choose to have just 1 I propose to replace HSTORE_GLOBAL_REGISTER with HSTORE_REGISTER_GLOBALLYand mantain backward compatibility, if we choose to have 2 separate settings I propose to find better naming, also in this case mantaining backward compatibility

Thanks for all the other response, now i'm starting to understand more.

niwinz commented 10 years ago

I propose to have DJANGO_HSTORE_REGISTER_TYPE = "global" or "connection" When "global" means unique=True and globally=True, and "connection" means unique=False and globally=False

That do you think about this approach?

niwinz commented 10 years ago

Also, DJANGO_HSTORE_REGISTER_TYPE can be also DJANGO_HSTORE_REGISTER_METHOD

nemesifier commented 10 years ago

How about DJANGO_HSTORE_ADAPTER_REGISTRATION which defaults to "global" but can be switched to "connection"?

niwinz commented 10 years ago

It's ok for me! Great name alternative ;)

nemesifier commented 10 years ago

done

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.44%) when pulling a4554b5151ff78397259bbd1afc3cf6905ecd72e on features/sqlalchemy into 45feb0535bf1045223e6f054b219d3bb6bb79d91 on master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.48%) when pulling 46a03f98fdf49e0e5d9663db171f9c1ffde89ad3 on features/sqlalchemy into 45feb0535bf1045223e6f054b219d3bb6bb79d91 on master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.48%) when pulling 46a03f98fdf49e0e5d9663db171f9c1ffde89ad3 on features/sqlalchemy into 45feb0535bf1045223e6f054b219d3bb6bb79d91 on master.

nemesifier commented 10 years ago

@niwibe i'm ready to merge

niwinz commented 10 years ago

Nice! Thanks!