-
Notifications
You must be signed in to change notification settings - Fork 6k
Convenience Functionality for BusinessOpeningHours #4861
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
Conversation
-check if the business is open at a given time -get the opening hours for a given day
Convenience Functionality for BusinessOpeningHours -check if the business is open at a given time -get the opening hours for a given day
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey. Thanks for your PR!
I gave it first quick look, but without actually reviewing the new logic. For starters, I focused on a few formalities and setup problems since you're apprantly strugging with this #4194 (comment). I hope that my comments help you.
Please also make sure to check the output of the pre-commit ci run in the "status checks" tab on the PR.
Please let me know if you need additional help with getting the tests running & pre-commit to pass - or when that's done and I should review properly :) Looking forward to it!
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
edit 4861.HEoGVs2mYXWzqMahi6SEhV.toml
|
Hello. Im having general problems to understand work flow here in git PR ) Please guide me) |
|
Hi I am going to give this a shot.
I do not quite get the question here, which requested changed did you not apply successfully? Yes the git actions will run on every commit. Doesn't really matter, the important part is that they don't fail on the last one.
The pre commit CI is imo pretty self explanatory, you apparently in the second PR also already fixed some issues there, so I am going to assume you understand that. You can also change the branch in this PR to your new one in your repo. We can ignore the Bot API test, and fix the chango/security analysis later. The important part to ensure you code is working correctly is the pre commit and the unit test. If you open any unit test run, and wait for it to load, you should have a huge wall of text. Scroll up to leave the flaky tests behind, until you see tests/test_business_classes.py:715: AssertionError. These are now the unit tests which failed, and they explain you where it failed and why (what it expected, what it got). You need to scroll up further to see them all, but you will get quickly an idea what the output format is. So either you need to fix the tests, or your code. Do that for all the tests, and see where that gets you. If you have more questions, make sure you read through the contrib guidelines and otherwise ask. Just here, or in our development group. |
|
On my pc i have this (python-telegram-bot) PS C:\Users\Aweryc\PycharmProjects\python-telegram-bot> pre-commit run -a Now go to CI steps) |
Convenience functionalty boh
# Conflicts: # changes/unreleased/4861.HEoGVs2mYXWzqMahi6SEhV.toml # src/telegram/_utils/datetime.py
# Conflicts: # changes/unreleased/4861.HEoGVs2mYXWzqMahi6SEhV.toml # src/telegram/_utils/datetime.py
|
Hello. I made a changes and now some unit tests are passed for different python versions, some ain't.
What should I do? |
|
Hello) I made a changes but again do not understand why tests are failing )
|
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for the updates!
Unfortunately I left quite a few comments again. I tried to be more specific to avoid new misunderstnadings. Please let me know if something is still unclear. I had a look at test_datetime this time due to the failing tests, but haven't gotten to test_business_classes yet …
Not waiting for flood control: Flood control exceeded. Retry in 25205 seconds
This is due to our test setup which uses real bots on Telegram - they get flood limits every now and then when a lot of tests are being run. You can ignore tests that failing due to this …
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
Bibo-Joshi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the late reply
|
I took the liberty to go ahead and implement my comments :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies for the overdue review! I left come docs and formatting comments. Tests looking very nice and complete btw :)
edit:
oops lost my comments, will add them back
6e951a9
into
python-telegram-bot:master
|
Thanks very much for the work you've put into this @Aweryc ! |
|
Hello, folks! Thank you all to give me an opportunity participate in such good product PTB! |
|
Oh no, that's tough to hear. hope you recover quickly and well! No need to apologize. |
|
thank you @Aweryc for the contribution. so sorry to hear about that. wishing you a speedy recovery 🙏 |
|
Thank you!) |
Convenience Functionality for BusinessOpeningHours
-check if the business is open at a given time
-get the opening hours for a given day