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

Implement EN Kantine Parsing #238

Merged
merged 3 commits into from
Mar 16, 2024
Merged

Implement EN Kantine Parsing #238

merged 3 commits into from
Mar 16, 2024

Conversation

Emily3403
Copy link
Contributor

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.

@ekeih
Copy link
Owner

ekeih commented Mar 14, 2024

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:

  • In 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.
  • After parsing the menu we get a Dict which uses an integer (the weekday) as its key. This Dict has no information about the actual date anymore, it just associates a weekday with a menu.
  • When the menu on the website is not updated for the next week yet, but we parse the menu at the end of the week, it can happen that we store this weeks menu for next weeks dates. This will be corrected as soon as the website is updated, but in the meantime we store and send wrong menus for next week.

I was wondering if this was also an issue in the old implementation, but I think it was not because f2e2634#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:

  • Get the actual date string from the h2 heading. (Maybe we should do some sanity checks here to verify that we found an actual date and that it is unique. 🤔)
  • Instead of using an integer as the key of the Dict use the date.
  • Then in 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 🚀

@Emily3403
Copy link
Contributor Author

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.

@ekeih
Copy link
Owner

ekeih commented Mar 15, 2024

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.

@Emily3403
Copy link
Contributor Author

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?

@ekeih
Copy link
Owner

ekeih commented Mar 16, 2024

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.

Copy link
Owner

@ekeih ekeih left a comment

Choose a reason for hiding this comment

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

Nice work, thanks again for your contribution! 🚀

@ekeih ekeih merged commit 9a12fdb into ekeih:main Mar 16, 2024
1 check passed
@ekeih
Copy link
Owner

ekeih commented Mar 16, 2024

image

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