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

Feature: Bumps #56

Merged
merged 7 commits into from
Apr 13, 2024
Merged

Feature: Bumps #56

merged 7 commits into from
Apr 13, 2024

Conversation

holedaemon
Copy link
Contributor

Hello!

Here's that PR we discussed. My friends and I have tested for a few nights & everything seems to work pretty well. Let me know if there's anything you want me to change.

Thanks

@jareddantis
Copy link
Collaborator

Hi @holedaemon! Thanks so much for the PR. Sorry for only getting to it today, I've been busy with work.

While I'm going through your code, could you walk me through how to use the feature and what the effects are when listening? I've yet to look deeper but I noticed mentions of a bump 'interval' and I'm not sure if that means normal playback is interrupted with bump tracks every X sec or something else.

Don't mind the failing check for now - looks like a permission issue with pushing the built bot image to GHCR, but nothing related to your code itself. You're the first external contributor to Blanco so I'll have to fix perms 😃

Thanks!

@jareddantis jareddantis self-requested a review March 30, 2024 05:54
@jareddantis jareddantis self-assigned this Mar 30, 2024
cogs/player/jockey.py Outdated Show resolved Hide resolved
cogs/player/jockey.py Show resolved Hide resolved
con.commit()

try:
cur.execute('''
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is more of a note to self, but in the future we might need to change how we handle column changes in existing schemas. The try-except pattern works well so far but I'm wary of it biting us in the future lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree. Out of curiosity, what was the reasoning behind the route taken for the database client, instead of an ORM or library?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@holedaemon Haha, good question. I had some familiarity with SQLAlchemy from past projects that used MariaDB and Postgres, but I've never tried it with SQLite.

Blanco actually started out as a cog from an old general-purpose bot called Rico. While I was spinning Blanco off into a separate bot, I also had to decide on a way to persist state, and a full RDBMS seemed overkill so I decided on SQLite. I read somewhere that SQLite's weak typing and other quirks with ALTER statements could affect how ORMs interface with it so I just never bothered with one and wrote migrations by hand.

With 1.0.0 I might finally start using SQLAlchemy. Still think it's a little overkill for a tiny project, but if it helps maintainability then why not.

utils/paginator.py Show resolved Hide resolved
cogs/bumps.py Outdated Show resolved Hide resolved
@holedaemon
Copy link
Contributor Author

holedaemon commented Apr 11, 2024

Hi @holedaemon! Thanks so much for the PR. Sorry for only getting to it today, I've been busy with work.

While I'm going through your code, could you walk me through how to use the feature and what the effects are when listening? I've yet to look deeper but I noticed mentions of a bump 'interval' and I'm not sure if that means normal playback is interrupted with bump tracks every X sec or something else.

Don't mind the failing check for now - looks like a permission issue with pushing the built bot image to GHCR, but nothing related to your code itself. You're the first external contributor to Blanco so I'll have to fix perms 😃

Thanks!

Howdy!

No worries, sorry about the delay on my part. I've also been busy with work and some other projects.

Sorry I didn't really provide any explanation as to how they work initially, I got a little bit ahead of myself. Bumps are off by default, and have to be toggled manually before use. The way the interval works is if N minutes have passed, then Blanco will play a bump after the current song ends, but before the next song starts -- there's no interruption. We've had bumps in rotation for a bit now and this has worked for us, but if you want that functionality to change at all, let me know. I'm going to start working on the requested changes, I'll keep you updated with anything I change. Appreciate your time!

@jareddantis
Copy link
Collaborator

Thank you for the revisions, LGTM!

@jareddantis jareddantis merged commit 86b0bb7 into deckardsworkspace:main Apr 13, 2024
1 of 2 checks passed
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.

2 participants