-
Notifications
You must be signed in to change notification settings - Fork 0
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
Various minor updates #14
Conversation
f14a3ee
to
683f505
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -56,8 +53,8 @@ def lock(self): | |||
|
|||
def release(self): | |||
self.cursor.execute( | |||
"SELECT {unlock_function}(%(lock_id)s)".format(unlock_function=self.unlock_function), | |||
{"unlock_function": self.unlock_function, "lock_id": self.lock_id}, | |||
f"SELECT {self.unlock_function}(%(lock_id)s)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's safe here - but would it be best practice (?) to use the psycopg2.sql functions for string building?
from psycopg2 import sql
f"SELECT {self.unlock_function}(%(lock_id)s)", | |
sql.SQL("SELECT {}(%(lock_id)s)").format(self.unlock_function)), |
where self.unlock_function
would be a sql.Identifier()
rather than a plain string?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One to ponder on - seems like a good idea.
Only slight downside is that this package would then technically require psycopg2 or psycopg directly (which currently we don't!)
Nothing too exciting for this PR, it's mostly dropping 3.7 support, adding 3.12 support, switching to ruff, and a bunch of other minor updates.
I've tested this locally with Django 5.1 and it's absolutely fine, but will leave it until Django 5.2 until we add to CI and confirm support.
Wanting to make sure we're keeping some of the packages we use/create up to date a bit more.