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

db: add methods for values associated with a plugin #1621

Merged
merged 1 commit into from
Aug 30, 2019

Conversation

deathbybandaid
Copy link
Contributor

@deathbybandaid deathbybandaid commented May 28, 2019

This aims to resolve issue #1501.

The one thing I was uncertain of is if we need to bother using .lower() like we do with channels.


@dgw edit: Closes #1501

@deathbybandaid
Copy link
Contributor Author

and depending on PR #1526 , I took the liberty of writing this snippet in advance for consistency. I was not sure if I should include it here, there, or wait until this, and #1526 are merged first.

def delete_plugin_value(self, plugin, key):
        """Deletes the value for a given key to be associated with the plugin."""
        plugin = Identifier(plugin).lower()
        session = self.ssession()
        try:
            result = session.query(PluginValues) \
                .filter(PluginValues.plugin == plugin)\
                .filter(PluginValues.key == key) \
                .one_or_none()
            # PluginValues exists, update
            if result:
                session.delete(result)
                session.commit()
        except SQLAlchemyError:
            session.rollback()
            raise
        finally:
            session.close()

@deathbybandaid
Copy link
Contributor Author

another other concern is if Identifier() even needs to be used here.

@deathbybandaid
Copy link
Contributor Author

Just dropping another note: should probably add to test_db.py

@dgw
Copy link
Member

dgw commented May 29, 2019

In brief:

  • Good start! Exciting to have an implementation of this.
  • Using .lower() on the plugin name is probably good for consistency, yes.
  • Casting the plugin name to Identifier is almost certainly unnecessary (it isn't one).
  • Pretend db: add methods to delete a channel or nick's key from database #1526 will be merged before this (because it probably will be) and include a delete_plugin_value method in this PR.
  • Adding tests is always valuable—always!
    • Per your IRC comments on this: As far as using sqlite directly goes, rewriting the DB tests to use SQLAlchemy instead is for a separate PR, and for this PR you should mimic the existing ones. (Opening a PR to switch the DB tests yourself is welcome of course!)

Longer thoughts:

Lacking any reasonable ideas for making the plugin argument optional (auto-detecting the calling module from within the db functions would require some possibly fragile introspection), I'll refrain from asking about that. I would love to have e.g. bot.db.set_plugin_value('key', True) "just work" without having to re-specify the plugin name, but… concerns about compatibility with alternative Python implementations and non-Linux platforms make me think doing so would be far more complicated than it's worth.

Besides, the existing set/get functions already require passing the nickname or channel, so the way this works now is parallel to how those work anyway. (Yes, I have thought about making the channel-value methods "automatic", too. Not worth it either, lol.)

@dgw dgw added this to the 7.0.0 milestone May 29, 2019
@deathbybandaid deathbybandaid force-pushed the db.pluginvalues branch 2 times, most recently from efd6be8 to 8425126 Compare May 29, 2019 11:57
@deathbybandaid
Copy link
Contributor Author

Identifier removed, lower() retained, tests added, squashed

test/test_db.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@deathbybandaid deathbybandaid left a comment

Choose a reason for hiding this comment

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

plugin names are not channels

test/test_db.py Outdated Show resolved Hide resolved
test/test_db.py Outdated Show resolved Hide resolved
test/test_db.py Outdated Show resolved Hide resolved
test/test_db.py Outdated Show resolved Hide resolved
@deathbybandaid
Copy link
Contributor Author

as per PR #1652 this has been updated

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Same consistency tweaks as #1526, as a start.

sopel/db.py Outdated Show resolved Hide resolved
sopel/db.py Outdated Show resolved Hide resolved
@deathbybandaid deathbybandaid force-pushed the db.pluginvalues branch 2 times, most recently from 2cc2ef2 to f0a4864 Compare July 5, 2019 12:09
@deathbybandaid deathbybandaid changed the title db: add methods for values associated with a plugin versus channel/nick db: add methods for values associated with a plugin Jul 9, 2019
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Just a few last consistency tweaks to docstrings/comments and we're done!

sopel/db.py Outdated Show resolved Hide resolved
sopel/db.py Outdated Show resolved Hide resolved
sopel/db.py Outdated Show resolved Hide resolved
@dgw dgw force-pushed the db.pluginvalues branch from 9990f65 to 502d8de Compare August 27, 2019 22:31
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Updated. Enjoy your trip, @deathbybandaid! (Well, as much as you can enjoy a work trip. 😼)

@dgw dgw merged commit e47ff3e into sopel-irc:master Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding plugin-based key-value store to SopelDB
3 participants