Closed Emily3403 closed 8 months ago
Hi @Emily3403,
this is great, thank you so much for your contribution! I was able to verify that your changes work and the chat messages it generates look good đ
Feel free to criticize code style and type hints.
Nothing wrong with either. If anything, we should add more type hints to the source code đ
I think I found one issue:
update_en_canteen()
we loop over the result of get_date_range()
which can return the dates of the current week or the next week.I was wondering if this was also an issue in the old implementation, but I think it was not because https://github.com/ekeih/OmNomNom/commit/f2e2634235fa8d9b2556ac9591c261ddfbcaf2eb#diff-db16d995ef351cde6d57cb6aaa888c1c2751bc4b6d6d72edc8c5ac24125f8dabL29 checked the actual date string on the website.
Thinking about possible solutions, my first idea is to change the parse_menu()
function a bit:
h2
heading. (Maybe we should do some sanity checks here to verify that we found an actual date and that it is unique. đ¤)update_en_canteen()
try to get menu based on the date string instead of the weekday.What do you think? Let me know if anything is unclear or if you think there is a mistake in my thoughts.
âšī¸ I just merged two major updates of the python-telegram-bot library to main and did some necessary code changes in frontend.py
for the upgrades. This should not affect any of your work but I wanted to mention it just in case.
Thanks again for your contribution! I am looking forward to get this released đ
Thank you for your detailed response đ
I've just implemented a fix for the issue you've spotted. Instead of using an int
for the key of the dictionary, it now uses datetime.datetime
parsed from the h2
heading. That should solve the problem of adding it to the next week.
Thanks for working on the fix đ
Unfortunately, I don't think it works as intended. menu.get(day)
always returns None
, so no data is stored in redis. I think this is because datetime is an object. So day
in update_en_canteen()
and date_time = datetime.datetime.strptime(date_str, '%d.%m.%Y')
in parse_menu()
are different objects. Even though they look identical they are different objects, and therefore using them as keys does not work.
I think I would use the string representation of the date as the key to avoid this issue.
Yep, I totally overlooked that. I kind of thought that datetime would work in a dict
but apparently not. Originally, I wanted to use the datetime to avoid any form of encoding issues with the str
. But if that is the only way, then so be it.
I've tested the parsing against last weeks menu by mocking the date in the get_date_range
function. It put data in the redis database, however upon querying it from the bot, it replied with an error. I think that has to do with weekends not being in the database.
How exactly should one handle weekends? Simply put the empty menu with all the fillers in the database?
Awesome, I can confirm that it works now đ
How exactly should one handle weekends? Simply put the empty menu with all the fillers in the database?
When there is no matching menu in redis the bot responds with Leider kenne ich keinen passenden Speiseplan. ...
. So on weekends it just does that.
it replied with an error
That you got an error during testing is most likely because you set yourself as admin via the OMNOMNOM_ADMIN
environment variable. In that case you get an error message whenever a user is causing an error, e.g. asking for an unknown canteen or day.
If another user queried the bot they would have gotten the Leider kenne ich keinen passenden Speiseplan. ...
message and you would have gotten the error message.
Hi there, inspired by #221 I decided to implement parsing for the EN Kantine. With my local setup, I am able to parse and query the menu for the current week. I'm not quite sure if there is a way to look at the menu of next week, however that could be implemented too.
A great deal of the code is inspired by f2e2634, especially
format_dish
. Thank you for mentioning it as it made the implementation a lot easier.Feel free to criticize code style and type hints. I've added them because it makes programming in my setup easier, however for consistency I totally see if they should be removed.