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

use python-sql instead of psycopg.sql for generating sql-queries #83

Conversation

r-peschke
Copy link
Member

No description provided.

@r-peschke r-peschke added the enhancement General enhancement which is neither bug nor feature label Apr 17, 2024
@r-peschke r-peschke added this to the 4.2 milestone Apr 17, 2024
@jsangmeister jsangmeister changed the base branch from main to feature/relational-db April 18, 2024 07:18
Copy link
Contributor

@jsangmeister jsangmeister left a comment

Choose a reason for hiding this comment

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

It would've been nice if you'd have separated the commits into one single "cleanup" commit and one commit with actual changes. This way, it is really hard to review.

Apart from that, I don't see many changes which are not simple reformattings. I noticed that there are still many usages of psycopg.sql in dev/tests/base.py, is this intended or do you plan to clean that up as well?


from psycopg import Cursor, sql
from sql import Column, Table # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the following section to the setup.cfg to not have to add # type: ignore everytime you import something from the sql package:

[mypy-sql]
ignore_missing_imports = true

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I did yesterday and now tried again, but the errors stayed.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@luisa-beerboom luisa-beerboom removed their assignment Apr 18, 2024
@r-peschke
Copy link
Member Author

It would've been nice if you'd have separated the commits into one single "cleanup" commit and one commit with actual changes. This way, it is really hard to review.

Apart from that, I don't see many changes which are not simple reformattings. I noticed that there are still many usages of psycopg.sql in dev/tests/base.py, is this intended or do you plan to clean that up as well?

It is intended to use some psycopg.sql, because python-sql has some limitations:

  • only dml-, but no ddl-support
  • only implementation for standard sql. Some special features of postgres are not implemented intentionally

IMO python-sql is helpful to write the usual dml-sql, for special cases we need the (harder) psycopg.sql

dev/setup.cfg Outdated Show resolved Hide resolved
dev/src/db_utils.py Outdated Show resolved Hide resolved
dev/src/python_sql.py Outdated Show resolved Hide resolved
@r-peschke r-peschke merged commit 572111f into OpenSlides:feature/relational-db Apr 19, 2024
1 check passed
@r-peschke r-peschke deleted the I13_python_sql_replaces_psycopg.sql branch April 19, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancement which is neither bug nor feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants