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

Move to SQLite #20

Merged
merged 17 commits into from
Mar 29, 2020
Merged

Move to SQLite #20

merged 17 commits into from
Mar 29, 2020

Conversation

eibex
Copy link
Owner

@eibex eibex commented Mar 26, 2020

Drop CSV in favour of SQLite.

Closes #19

@eibex
Copy link
Owner Author

eibex commented Mar 26, 2020

cc @arbaes
Sorry for the mention, but thought of asking for your input if you have some time in the next days.
I was wondering what were your thoughts about this to implement SQLite.

bot.py is still a mess that needs to be redone, but I am quite content with rldb.py readability (which is the old rlightfm.py).

Do you think I need to change anything before working on a migration script and merging?

@arbaes
Copy link
Contributor

arbaes commented Mar 26, 2020

I can try to look into it tomorrow, but I'll have way more time this weekend ! getting back to you asap

@eibex eibex marked this pull request as ready for review March 26, 2020 23:55
@eibex
Copy link
Owner Author

eibex commented Mar 27, 2020

I think I made a mess during the merge into the sqlite branch. The commands work fine. But when I edit an embed the embed title is not given, and the botcolor is not respected either. I will try and fix it later today when I have more time.

@eibex
Copy link
Owner Author

eibex commented Mar 27, 2020

Actually I do not see the code for it in master either. If you edit a reaction-role message without an embed, and pass arguments in the edit command to add an embed, is a title and botcolour given?

@eibex
Copy link
Owner Author

eibex commented Mar 27, 2020

It was a much faster fix than I had anticipated. I believe this PR is merge-ready, but I will wait for your comments since a lot of code is changing. ^^

@arbaes
Copy link
Contributor

arbaes commented Mar 27, 2020

I'll promise I'll be testing this tomorrow !

@eibex
Copy link
Owner Author

eibex commented Mar 27, 2020

No worries, there's no rush :)

I will be squashing the commits on merge, I am not bothering to force push right now.

Copy link
Contributor

@arbaes arbaes left a comment

Choose a reason for hiding this comment

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

Hello, I left a few comments with my opinion on this :)

rldb.py Outdated

def commit(self):
db.execute(f"CREATE TABLE '{self.message_id}' ('reaction' NVCARCHAR, 'role_id' INT);")
db.execute(f"INSERT INTO 'channels' ('message_id', 'channel') values('{self.message_id}', '{self.target_channel}');")
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating a table for each message seems way too overkill for me. Why not creating a single table for every messages, and another table for the reaction/roles with a foreign key linking them to their message instead ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

So, I should re-implement what id.csv is doing with foreign IDs?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think I can do this with only two tables instead of 3 actually. I will push some changes soon.

rldb.py Outdated

db.execute("CREATE TABLE IF NOT EXISTS 'channels' ('message_id' INT, 'channel' INT);")

embeds_creation = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

My bad for that, but maybe "embed" isnt really the right word anymore now :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, pretty weird to keep a global variable if you want to uses classes IMO, but that's just personal preferences

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I could use a different word other than embed.

This might sound quite dumb, but how would I access an instance of a class between different steps without a global dict?

Right now to get the instance of the class I'm interested in is by using the key "user_channel" in the dict.

Without a dict wouldn't the bot lose the possibility of managing multiple simultaneous reaction-role messages?

@eibex
Copy link
Owner Author

eibex commented Mar 28, 2020

Thanks, I will start working on these.

@eibex
Copy link
Owner Author

eibex commented Mar 28, 2020

I think I fixed all your concerns, bar the global dict.

@eibex
Copy link
Owner Author

eibex commented Mar 28, 2020

I will be merging this branch in a few hours if you don't have anything against it. :)

@eibex eibex merged commit 2fc2b89 into master Mar 29, 2020
@eibex eibex deleted the sqlite branch March 29, 2020 12:11
eibex added a commit that referenced this pull request Aug 23, 2020
eibex added a commit that referenced this pull request Aug 23, 2020
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.

Move to SQLite
2 participants