Skip to content

Conversation

@AdetonaMichael
Copy link

References and Issues/PRs
None

What does this implement/fix? (Changes)

  • Added icon and a scheduler for the next search. the scheduler is set to 5 seconds by default.
  • Added windows notification features to tell result and time for the next search

Any other comments?
No

…added icon and a schedulaer for the next search. the scheduler is set to 5 seconds by default
import time

from bs4 import BeautifulSoup
from win10toast import ToastNotifier
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the duplicate import

requests==2.22.0
beautifulsoup4~=4.9.3
beautifulsoup4~=4.9.3
win10toast
Copy link
Owner

Choose a reason for hiding this comment

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

  1. Its always a good practice to have the version in the requirements, its possible libraries introduce breaking changes in newer versions which might break the application.
  2. win10toast is a windows binary, so I am not even able to run this code on a mac, you can add something like this win10toast==0.9; platform_system == "Windows", which would only install this on windows devices

Copy link
Author

Choose a reason for hiding this comment

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

alright Got it

@Chelsea31
Copy link
Owner

Thank you for the PR @AdetonaMichael!
The PR mostly looks good, but there are a couple of things which need some work. But one big change would be to figure out how to abstract windows toaster notification since we want to our code to run on every device, regardless of OS.
Feel free to reach out to me if you need any help in designing or figuring out the solution.

Copy link
Owner

@Chelsea31 Chelsea31 left a comment

Choose a reason for hiding this comment

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

Changes are requested for making code agnostic of OS, or even inclusive of all OS
You can have toaster notification for windows, and just leave a simple print for other OS's
I am eager to see what you come up with! 😄

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