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 function for registering as array #19

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

mzpqnxow
Copy link

@mzpqnxow mzpqnxow commented Feb 2, 2021

Per issue #9, the implementation suggested by @rmihael

Tested this and it works on CPython 3.7. I now get back an array of strings as opposed to an array of characters

You need to explicitly call this function with an engine so it can do the OID introspection on the database

@mzpqnxow mzpqnxow mentioned this pull request Feb 2, 2021
@mahmoudimus
Copy link
Owner

@mzpqnxow thank you so much! I will merge and release shortly.

@mzpqnxow
Copy link
Author

mzpqnxow commented Feb 6, 2021

Happy to help, thanks for your work on this project, it’s been very helpful to me

If you can, please throw a note in here and/or close it out once you’ve released a new package so I know that I can clean up my constraints.txt :)

@mahmoudimus
Copy link
Owner

For sure I will merge and update by tomorrow evening at the latest (Mon Feb 8th).

@mzpqnxow
Copy link
Author

@mahmoudimus no rush, but figured I would ping you. I figure you might have just been busy and forgot :)

@mahmoudimus mahmoudimus merged commit 613252a into mahmoudimus:master Mar 2, 2021
@mahmoudimus
Copy link
Owner

@mzpqnxow thank you so much! I have merged + release 1.8 coming soon. Really appreciate your patience.

@mahmoudimus
Copy link
Owner

@mzpqnxow Release cut for version 1.8.0 and published to pypi

@fake-name fake-name mentioned this pull request May 31, 2021
@fake-name
Copy link

fake-name commented May 31, 2021

This PR breaks any non-psycopg adapter uses.

Additionally, the new psycopg2 dependency isn't made explicit anywhere (requirements.txt, setup.py, etc) so it will happily install in a context without psycopg2, and then explode on import.

@mzpqnxow
Copy link
Author

mzpqnxow commented Jun 2, 2021

This PR breaks any non-psycopg adapter uses.

Additionally, the new psycopg2 dependency isn't made explicit anywhere (requirements.txt, setup.py, etc) so it will happily install in a context without psycopg2, and then explode on import.

You're right @fake-name, thanks for pointing it out. For now I can add psycopg2 to the install_requires. Another option is to replace SQLAlchemy with SQLAlchemy[postgresql] but I'm not sure if that's guaranteed to work correctly (e.g. if there are other PostgreSQL backends available for SQLAlchemy

I'll send a new PR for this

@mzpqnxow
Copy link
Author

mzpqnxow commented Jun 2, 2021

Added PR #22, thanks again @fake-name for catching this

@mzpqnxow
Copy link
Author

mzpqnxow commented Jun 2, 2021

This PR breaks any non-psycopg adapter uses.

Additionally, the new psycopg2 dependency isn't made explicit anywhere (requirements.txt, setup.py, etc) so it will happily install in a context without psycopg2, and then explode on import.

I should clarify, though I alluded to it (kind of) in my previous note- @fake-name do you think it's necessary to provide the ability for users to use sqlalchemy-citext without requiring them to have psycopg2? Another way to fix this is to just wrap the import and the call in a try/except ImportError clause

Curious about your take on this as well @mahmoudimus

The only case where this approach would be desired/useful is one where (for whatever reason) the user isn't able to get psycopg2 to install on their system- maybe because libpq is not present and for some reason is a hardship to install/build. I don't know if that's a condition that exists in practice

@fake-name, are there PostgreSQL backends for SQLAlchemy that are pure Python? Or does every PostgreSQL interface for Python require libpq? My question is ultimately "is simply adding psycopg2 to requirements.txt and setuptools good enough for everyone?"

@fake-name
Copy link

fake-name commented Jun 3, 2021

I mean, I use pypy extensively and it doesn't have a port of psycopg2. You instead use psycopg2cffi, which is a drop-in replacement. There's also pg8000, which is a pure-python DBAPI driver implementation.

Additionally, I have seen people who are using pg8000 for legal reasons, as their company policy is no GPL/LGPL code allowed. pg8000 is MIT licensed, as is this extension (and SQLAlchemy).

SQLAlchemy supports all of the above, so hard requiring any specific driver breaks all the others when they might have been working. I think the correct implementation here would be to determine how SQLAlchemy does it's imports, and just use that.

Additionally, the new psycopg2 dependency isn't made explicit anywhere (requirements.txt, setup.py, etc) so it will happily install in a context without psycopg2, and then explode on import.

For clarification, the fact that it's not made explicit just makes the underlying issue more annoying to debug, it doesn't solve the actual problem (dependency on a package that's not available on the pypy/whatnot).

@mahmoudimus
Copy link
Owner

Fwiw I agree, we shouldn't depend on a particular driver. I will see if I can get to this tonight and propose a PR

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.

3 participants