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.

Sign up for GitHub

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

Jump to bottom

db: raw sql functionality and session object api #1811

Merged
merged 5 commits into from
Jan 26, 2020
Merged

db: raw sql functionality and session object api #1811

merged 5 commits into from
Jan 26, 2020

Conversation

RustyBower
Copy link
Contributor

No description provided.

@dgw dgw added this to the 7.0.0 milestone Jan 24, 2020
@dgw dgw added the Tweak label Jan 24, 2020
@RustyBower RustyBower force-pushed the raw_sql_fix branch 3 times, most recently from 9e62c60 to cca439b Compare January 24, 2020 04:17
dgw added 3 commits January 23, 2020 22:54
@dgw
Toss notes about non-SQLite DBs + old plugins all over the place
9d7189f 
We can make SQLite work pretty much identically to how it did before,
but there are still some things we can't fix. Differing DBAPI behavior
is one of those things, for example placeholders in raw SQL.

Instead of trying to patch over all the little differences and obviating
the purpose of using an ORM in the first place, we'll simply point out
in as many places as possible that old plugins might not work right when
used with a non-SQLite database.

Besides, it's pretty impossible to patch over all the differences unless
we're going to turn Sopel's DB API into an ORM in its own right.
@dgw
db: warnings & more documentation for connect() vs. session()
82cd080
@dgw
db: log when db.connect() is called if db_type isn't sqlite
6db7f26 
This is useful information to have in the log in case a user reports a
problem with a plugin, because a plugin that uses this might have errors
on non-sqlite DB types due to connection object behavior differences. We
want to know if a plugin is using this method.
@dgw dgw requested a review from Exirel January 24, 2020 05:58
@dgw
Copy link
Member

dgw commented Jan 24, 2020

Right, I spammed warnings all over the place. Hopefully Exirel will have time to look in the next few days.

Exirel
Exirel requested changes Jan 24, 2020
View reviewed changes
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

I like the idea of connect() and session() for backward compatibility reasons. Good call!

sopel/db.py Show resolved Hide resolved
sopel/db.py Show resolved Hide resolved
sopel/db.py Show resolved Hide resolved
sopel/db.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member

dgw commented Jan 24, 2020

Updated the warning in db.connect(); it now looks like this:

[2020-01-24 16:47:33,258] sopel.db             INFO     - Raw connection requested when 'db_type' is not 'sqlite':
Consider using 'db.session()' to get a SQLAlchemy session instead here:
  File "/usr/local/lib/python3.6/dist-packages/sopel_modules/github/webhook.py", line 49, in setup_webhook
    conn = sopel.db.connect()

@dgw dgw requested a review from Exirel January 24, 2020 22:56
@dgw dgw merged commit dfb9350 into sopel-irc:master Jan 26, 2020
@dgw dgw mentioned this pull request Jan 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgw dgw dgw left review comments

@Exirel Exirel Exirel approved these changes

Assignees
No one assigned
Labels
Tweak
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@RustyBower @dgw @Exirel