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

feat implement subjects cache #8

Merged
merged 2 commits into from
Jun 9, 2024
Merged

Conversation

Patai5
Copy link
Owner

@Patai5 Patai5 commented Jun 9, 2024

Implements a subject names cache that stores subjects extracted from grades and schedules.


This resolves the need to have the subjects hard-coded as constants. They are now automatically extracted from schedules and grades and are stored into a cache.

Hi @Vojtak42, if you have time and will, would please look at this PR? It's pretty big, so it's pretty hard for a review, but I will comment out the most important changes :) Many thanks!
I can't currently add you as a reviewer since you are not a collaborator in this repo. I have sent you an invite ;)


Context:

  • Subjects have a full and a short name
    • Full name can be extracted from both grades and schedule
    • Short name can only be extracted from schedule
    • As such a need to cache subjects between the two methods of scraping arises, so we can display the short name where we want

Ideally we would only extract this from the schedule then, but an edge case where there would be a grade for a subject that was not yet seen in the schedule would break it. So we have to scrape it from both places.
If a short name cannot be scraped, the full name is used as a fallback.

@Patai5 Patai5 requested a review from Vojtak42 June 9, 2024 18:20
@Patai5
Copy link
Owner Author

Patai5 commented Jun 9, 2024

I've added the comments for the important parts so it's easier to review.

The most important file for review is the subjects_cache.py:
https://github.com/Patai5/BakaBot/blob/f1e8e02f95682211dfe484882a9e9dc9c8a06343/src/bakabot/core/subjects/subjects_cache.py

You can pretty much ignore everything else, as this is the only new thing and others are just some small implementations of this :)
Thanks a lot!

@Vojtak42
Copy link
Collaborator

Vojtak42 commented Jun 9, 2024

I am afraid that's far beyond my coding capabilities. I can mostly undestand what the code does but it is not enough to find mistakes. Honestly i have never looked up python syntax yet.

@Patai5
Copy link
Owner Author

Patai5 commented Jun 9, 2024

That's okay :D I've tested it, and it works well (or at least I hope so :P) so it's only if you find some obvious mistakes.

Don't worry if you can't fully read it, that's only a sign of the fact that it's not coded well, I would be glad if you could point those parts out :)
If it looks good to you (or at least as much as it can), you can approve the changes 👍 I mean, there isn't really anyone else for a review so that's that 😄

@Vojtak42
Copy link
Collaborator

Vojtak42 commented Jun 9, 2024

I would also add the url (for example: https://gymkh.bakalari.cz) to the .env for easier modification. That's fast change.

@Patai5
Copy link
Owner Author

Patai5 commented Jun 9, 2024

Yeah, I will add that outside of this PR to keep the history clean. Will push it to main now 👍 Thanks for the suggestion!

@Vojtak42 Vojtak42 merged commit dfd07cb into main Jun 9, 2024
@Vojtak42 Vojtak42 deleted the feat-implement-subjects-cache branch June 9, 2024 19:17
@Vojtak42
Copy link
Collaborator

Vojtak42 commented Jun 9, 2024

that's only a sign of the fact that it's not coded well
I'm pretty sure that's not the reason.😄

@Patai5
Copy link
Owner Author

Patai5 commented Jun 9, 2024

Thanks for looking into it 🙏

I've pushed the fix for the hard-coded bakalari URL to main. You can use the bakalariUrl env variable now :)
I will probably rework the lesson times to be dynamic next. And probably some refactoring afterward 🤞

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.

None yet

2 participants