Business info #4183
Business info #4183
Conversation
Add the classes `BusinessOpeningHours` and `BusinessOpeningHoursInterval` and the field `business_opening_hours` to the class `Chat`.
|
Ready for review. |
There was a problem hiding this comment.
Pretty much perfect overall. Just left some comments mostly regarding naming/doc inconsistencies.
Also I just realized that you may need TG Premium to test opening/closing_minute related review comment so you can just ignore that if you don't have premium and i'll handle that later.
|
Thanks for the review! With regard to Finally, we could make a utility (day, hour, minute) = parse_hour(opening/closing_hours)
# where day in [0, 1, .., 6]
# hour in [0, 1, .., 24],
# minute in [0, 1, .., 60]But also users can calculate it themselves, since it's simple math operations. |
|
I just tested what values are returned by the API and it's clear that those times are representing the minute of the week. Here's the results: I'll update the doc to further explain how those numbers are achieved. Now the utility function - I like your idea but we can split it into two different properties -
Another idea - (this may be a little out of scope) - We can have another function BusinessOpeningHours.convert_to_timezone(timezone: datetime.tzinfo) -> BusinessOpeningHoursIntervalAdvantage is that its easier to schedule jobs and reduces potential errors. cc @Bibo-Joshi |
|
@aelkheir so @Bibo-Joshi and I discussed internally and decided to not pursue |
Funnily enough i was just here to say that I don't know of a universal time representation that may fit all users, so we could let them use thetuple of ints and produce a representation to their liking. I'll do it here and get it over with. But not untill tomorrow night hopefully :). |
|
About the function A possibility i thought about was to get hours offset from BusinessOpeningHoursInterval(
opening_minute=opening_minute + offset * 60,
closing_minute=closing_minute + offset * 60
)with special handling to keep the minutes in the range But this will require us to use pytz (?), and I see it's optional. |
There was a problem hiding this comment.
Nice PR, thank you very much!
Regarding convert_to_timezone & other auxiliary functionality: representing an offseted-time in form of BusinessOpeningHoursInterval doesn't make much sense to me, tbh.
What I could think of is a method BOH.get_opening_hours_for_day(dtm.date, tzinfo=None) -> tuple[tuple[dtm.datetime, dtm.datetime], …]. I.e. you provide a specific day and optionally a timezone and get as return value a sequence of the opening intervals for that day in the desired timezone. Making the user provide a timezone would also lift the requirement on our side to build the tzinfo object. We'd only have to deal with the case where the user does not pass a timezone and we'd fall back to time_zone_name. Until zoneinfo becomes usable for us (once we drop py3.8 I would personally just leave the datetime tz-naive …
Another thought that I had is BOH.is_open(dtm.datetime) -> bool, i.e. a method that would allow you to check if the business is open at that specified time. However I see that tz-dealing is more difficult again here, if we can't build a tzinfo object from time_zone_name, which brings me to believe that this should also wait until we drop py3.8
Since this functionality is very new and this is only the initial implementation on our end, I would not like to add additional dependencies for these, still rather minor, convencience methods. Once could ofc raise RuntimeError if pytz is not availbale, but that also seems like rather much effort for little gain …
|
Okay, I'm fine with waiting until we drop py 3.8 to implement these functions. |
|
October (python 3.8 end of life) is not so far away. By then pytz could be droped entirely in favor of zoneinfo. Thanks very much team for the opportunity <3 |
|
@aelkheir Thanks for the contribution! |
Partially closes #4179
Information about Business Accounts [Taken by @aelkheir]
Check-list for PRs
.. versionadded:: NEXT.VERSION,.. versionchanged:: NEXT.VERSIONor.. deprecated:: NEXT.VERSIONto the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)CSI standard <https://standards.mousepawmedia.com/en/stable/csi.html>__Added myself alphabetically toAUTHORS.rst(optional)__all__sStability Policy <https://docs.python-telegram-bot.org/stability_policy.html>_ in case of deprecations or changes to documented behaviorIf the PR contains API changes (otherwise, you can ignore this passage)
self._id_attrsand corresponding documentation__init__acceptsapi_kwargsas kw-only