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

Choose between FrugalFeeds and Couponese (issue #204) #205

Merged
merged 6 commits into from
Jul 5, 2024

Conversation

tj-dodd
Copy link
Contributor

@tj-dodd tj-dodd commented Jun 30, 2024

This gives the user the ability to choose between FrugalFeeds and Couponese through a source optional argument. I ended up making it so the bot's default behaviour is to give the user a randomly generated combined list of coupons. I changed the hyperlink for the embed to FrugalFeeds, as there's a more comprehensive collection of coupon codes there.

@tj-dodd tj-dodd changed the title Choose between FrugalFeeds and Couponese (#204) Choose between FrugalFeeds and Couponese (issue #204) Jun 30, 2024
Copy link
Member

@49Indium 49Indium left a comment

Choose a reason for hiding this comment

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

Firstly, thank you for contributing! This is a great improvement and should help reduce the trial and error needed when getting pizza!

Overall the command adjustments appear to to be working great. I like how you have structured _get_coupons_from_page and the idea of shuffling coupons each time. There are some small adjustments I think would be nice which I've attached with the code. Some of these comments are up to your discretion; they are all relatively minor and could be argued either way if they are needed.

uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
@tj-dodd
Copy link
Contributor Author

tj-dodd commented Jul 1, 2024

Just committed addressing your changes - if there's any issues let me know.

I ended up reverting back to how you were originally parsing dates. My additions were unnecessary I think - to be transparent, I'm not completely sure what I was trying to do, but I believe at one point I mistakenly assumed I had to do some extra parsing as the month formatting on FrugalFeeds is slightly different and I was encountering a couple of weird dates (e.g. soon!). I think leaving the date as a string if it's unable to be parsed is a much better implementation. There's really only 1-2 ambiguous dates in the FrugalFeeds list and the format is slightly different to Couponese, but the user still receives more than enough information even if the date is passed through as an unformatted string. If you'd rather it be formatted and the bot use my original implementation, let me know.

I'm still not certain if giving users a random sample of discount codes is the best way to approach things, as people might get unlucky (e.g. bot giving them 5 different mudcake discounts, excellent!), but I guess it's on the user to make use of the keywords function, and if you're getting decent results then I guess it's fine.

@tj-dodd
Copy link
Contributor Author

tj-dodd commented Jul 1, 2024

nevermind, I think most recent commit is the best way to do the formatting

@tj-dodd tj-dodd requested a review from 49Indium July 1, 2024 09:52
@tj-dodd
Copy link
Contributor Author

tj-dodd commented Jul 3, 2024

Some small changes:

  • Bot now hints user to change source if their keywords don't return any results on a single website
  • FrugalFeeds went rogue and started advertising pizza hut deals on their dominos page, these have been filtered out
  • Fixed FrugalFeeds formatting for discounts with multiple codes
  • Moved hyperlinks down to description area, bot now links both FrugalFeeds and Couponese if both are selected

Copy link
Member

@49Indium 49Indium left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes. It's annoying that FrugalFeeds has included different codes.
I'm unsure how much I like the structure of the large for loop (iterating over range(0, NUMBER_WEBSITES) within the main command function. Looping over a list of urls in _get_coupons or _get_coupons_from_page might be a better alternative.

uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
@tj-dodd
Copy link
Contributor Author

tj-dodd commented Jul 5, 2024

An update:

  • Requested changes implemented
  • Now checks for and removes duplicate codes
  • A good amount of restructuring in __get_coupons_from_page, _get_coupons, and DominosCoupons

Copy link
Member

@49Indium 49Indium left a comment

Choose a reason for hiding this comment

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

This is getting close to ready. I like the duplicate checking! A few small changes would be nice to make the typing more readable and reduce repetition.

uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
uqcsbot/dominos_coupons.py Outdated Show resolved Hide resolved
@tj-dodd
Copy link
Contributor Author

tj-dodd commented Jul 5, 2024

Changes done 👍

@49Indium 49Indium merged commit f22543a into UQComputingSociety:main Jul 5, 2024
3 checks passed
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