search

reminder-bot / reminder-rs

**MOVED** Please go to https://gitea.jellypro.xyz/jude/reminder-bot
https://gitea.jellypro.xyz/jude/reminder-bot
GNU Affero General Public License v3.0
48 stars 10 forks source link

[WIP; broken] Use event_parser crate for natural language date parsing #12

Open allquixotic opened 1 year ago

allquixotic commented 1 year ago

Removing our dependency on forking and execing Python to call on the dateparser library, we can use the event_parser Rust crate which provides similar functionality (but not equivalent).

One limitation of event_parser that is immediately obvious vs. python-dateparser is the lack of non-English language support.

At this point, the event_parser code successfully produces an accurate UNIX timestamp and returns it from the time_parser::natural_parser() for a few simple English-language test cases, like in 20 minutes or 20 minutes ago.

What doesn't work is something deep within reminderbot. You can find a panic stack trace here: https://gist.github.com/allquixotic/c908e043dff4ff9219463209756d0618

This stack trace is from /remind time: in 20 minutes content: blah, and I verified through println!() in time_parser::natural_parser() that a valid UNIX timestamp is being returned to the caller.

This PR contains several crate version bumps, so that may have something to do with the crash.

JellyWX commented 1 year ago

Error seems to be from this query: https://github.com/reminder-bot/reminder-rs/blob/a3aa234b2e8178d92c9cbd80c65b47ef089b9d70/src/models/reminder/builder.rs#L68

I don't know why I even wrote it to query directly like that. Not sure exactly why it's failing, though. Are you running MySQL 8 on your local?

allquixotic commented 1 year ago

Error seems to be from this query:

https://github.com/reminder-bot/reminder-rs/blob/a3aa234b2e8178d92c9cbd80c65b47ef089b9d70/src/models/reminder/builder.rs#L68

I don't know why I even wrote it to query directly like that. Not sure exactly why it's failing, though. Are you running MySQL 8 on your local?

I'm running MariaDB 10.6.x on Ubuntu 22.04. Sorry; the instructions didn't specify MySQL specifically (vs. MariaDB), and there are quite some differences between the two products now. I had to adjust that query for MariaDB support, but I'm not even sure if my fix was correct, or if I should abandon MariaDB support and just install Oracle MySQL.

I used MariaDB because an up-to-date version is available in the Ubuntu package manager without a third-party package; and because of the preferential licensing and support situation for MariaDB vs. MySQL (in my opinion).

JellyWX commented 1 year ago

Ah okay, really it'd be good to avoid totally non-standard SQL stuff that will break Maria. I can have a look at modifying the query there maybe, or removing it entirely

allquixotic commented 1 year ago

I also had issues with the migration queries that are used to create the DB. I might open a separate PR about that.

One issue I recall was RENAME COLUMN only exists in MariaDB >= 10.5, and I was running 10.3 initially. I did an Ubuntu distro version upgrade to get to 10.6, and that problem went away, but MariaDB 10.3 was released in 2017, and is frequently in use with Debian and Ubuntu LTS releases from 2020 and earlier.

There was also an issue with a column being created with a default value of NULL for a timestamp column; I had to add NULL before the keyword TIMESTAMP to fix that. And the list goes on...

JellyWX commented 1 year ago

https://github.com/reminder-bot/event-parser

JellyWX commented 1 year ago

I also had issues with the migration queries that are used to create the DB. I might open a separate PR about that.

One issue I recall was RENAME COLUMN only exists in MariaDB >= 10.5, and I was running 10.3 initially. I did an Ubuntu distro version upgrade to get to 10.6, and that problem went away, but MariaDB 10.3 was released in 2017, and is frequently in use with Debian and Ubuntu LTS releases from 2020 and earlier.

There was also an issue with a column being created with a default value of NULL for a timestamp column; I had to add NULL before the keyword TIMESTAMP to fix that. And the list goes on...

Sorry, I didn't see this message. MySQL is a bit of a curse: realistically, I'd love to move to Postgres, but the amount of effort that requires is immense. I'd accept a PR to improve SQL compatibility, but it does need to remain compatible with MySQL 8 just because I have other software running on the server, and I don't want to run a third database server.

I don't know the exact compatibility issues really. I can spin up a VM and go through and test some of this stuff