Issue 423091003: tryton-env: Add timezone on cronjobs
Descriptiontryton-env: Add timezone on cronjobs
issue9436
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use zoneinfo or dateutil as timezone source and use a configuration entry #
Total comments: 13
Patch Set 3 : Add doc + better testing + use tz from environment #
Total comments: 12
Patch Set 4 : Add timezone in tools and timezone field to cron job #
Total comments: 38
Patch Set 5 : Fix remarks #
Total comments: 17
Patch Set 6 : Fix remarks #
Total comments: 14
Patch Set 7 : Fix remarks #Patch Set 8 : Do tz conversion in next_call only #
Total comments: 1
Patch Set 9 : Adapt tests #Patch Set 10 : Add UTC fallback when the environment variable is a non existing TZ #
Total comments: 4
Patch Set 11 : More compact handling of non existing IANA key #
Total comments: 7
Patch Set 12 : Remove duplication #Patch Set 13 : Reuse active voice #
Total comments: 20
Patch Set 14 : Add test on UTC fallback #
Total comments: 1
MessagesTotal messages: 57
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/421091003/trytond/setup.py File trytond/setup.py (right): https://codereview.tryton.org/423091003/diff/421091003/trytond/setup.py#newco... trytond/setup.py:75: tests_require = ['pillow'] We should add pytz to tests require as the test are now using this optional dependency. https://codereview.tryton.org/423091003/diff/421091003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/421091003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:77: help="Used to compute the next call") help text should end with a dot
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:28: _DISPLAY_TZ = config.get('database', 'timezone', default='UTC') I wonder if this information shouldn't be displayed in the form so that people in other TZ will know which hour the field hour mean.
Sign in to reply to this message.
ERROR: /tmp/reviewbot-2qbks07a/trytond/trytond/ir/cron.py Imports are incorrectly sorted and/or formatted. ERROR: /tmp/reviewbot-2qbks07a/trytond/trytond/tests/test_ir.py Imports are incorrectly sorted and/or formatted. URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
Missing documentation. https://codereview.tryton.org/423091003/diff/417111003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/423091003/diff/417111003/trytond/CHANGELOG#newc... trytond/CHANGELOG:1: * Add timezone handling on cron jobs For me we must add a general timezone for the server. https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:10: from dateutil.tz import gettz I think it is better that gettz is renamed into ZoneInfo https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:10: from dateutil.tz import gettz This may be useful for others so it will be better to have it in tools https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:28: _DISPLAY_TZ = config.get('database', 'timezone', default='UTC') For me the timezone is not related to database. But indeed I'm wondering if we should not just store the original timezone of the system before switching to UTC in __init__.py This would avoid to create a new configuration option. https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:28: _DISPLAY_TZ = config.get('database', 'timezone', default='UTC') On 2022/05/05 08:02:01, nicoe wrote: > I wonder if this information shouldn't be displayed in the form so that people > in other TZ will know which hour the field hour mean. Indeed it may be good to add it to the views using a Function field. https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/tests/... File trytond/trytond/tests/test_ir.py (right): https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:393: class IrCronTestCase(ModuleTestCase): I do not see the point to test the module again. https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:424: def test_compute_next_call_dst_summer(self): I do not see what part of the Tryton's code this is testing.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/417111003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/423091003/diff/417111003/trytond/CHANGELOG#newc... trytond/CHANGELOG:1: * Add timezone handling on cron jobs On 2022/05/05 12:28:37, ced wrote: > For me we must add a general timezone for the server. Shouldn't it be that we're storing the initial timezone setting and that we're using it for cron job computations? https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:10: from dateutil.tz import gettz On 2022/05/05 12:28:37, ced wrote: > This may be useful for others so it will be better to have it in tools Done. https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:28: _DISPLAY_TZ = config.get('database', 'timezone', default='UTC') On 2022/05/05 12:28:37, ced wrote: > For me the timezone is not related to database. > But indeed I'm wondering if we should not just store the original timezone of > the system before switching to UTC in __init__.py > This would avoid to create a new configuration option. Done. https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/tests/... File trytond/trytond/tests/test_ir.py (right): https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:393: class IrCronTestCase(ModuleTestCase): On 2022/05/05 12:28:37, ced wrote: > I do not see the point to test the module again. Because this part uses a setup that change the configuration. https://codereview.tryton.org/423091003/diff/417111003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:424: def test_compute_next_call_dst_summer(self): On 2022/05/05 12:28:37, ced wrote: > I do not see what part of the Tryton's code this is testing. I wanted to test Cron.run() but I had some issues to setup a mock for datetime.datetime.now so I opted to test compute_next_call. But since I wrote the code I found a way to mock the now call so I am back to testing the feature.
Sign in to reply to this message.
ERROR: /tmp/reviewbot-jt9s8b6j/trytond/trytond/__init__.py Imports are incorrectly sorted and/or formatted. URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/411131003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/CHANGELOG#newc... trytond/CHANGELOG:1: * Add timezone handling on cron jobs I would say: compute cron job scheduling using server timezone https://codereview.tryton.org/423091003/diff/411131003/trytond/doc/topics/cro... File trytond/doc/topics/cron.rst (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:16: The timezone used for the "hour" computation of occurences is the timezone for the schedule computation comes from the ``TZ`` environment variable. https://codereview.tryton.org/423091003/diff/411131003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:17: of the `TZ` environment variable. Use double ticks and run the check of the doc. https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/__init... File trytond/trytond/__init__.py (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/__init... trytond/trytond/__init__.py:9: from trytond.config import config I do not like to import config in the __init__. I see no problem to store the timezone as global variable. https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/tests/... File trytond/trytond/tests/test_ir.py (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:389: class IrCronTestCase(ModuleTestCase): I still do not understand why testing twice the module. https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/tools/... File trytond/trytond/tools/__init__.py (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/tools/... trytond/trytond/tools/__init__.py:10: from dateutil.tz import gettz as ZoneInfo I think it will be better to have a time submodule which provide more feature like the UTC and server timezone. It will also good in the future to provide the result of https://docs.python.org/3/library/zoneinfo.html#zoneinfo.available_timezones
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/411131003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/CHANGELOG#newc... trytond/CHANGELOG:1: * Add timezone handling on cron jobs On 2022/05/09 21:02:38, ced wrote: > I would say: compute cron job scheduling using server timezone Done. https://codereview.tryton.org/423091003/diff/411131003/trytond/doc/topics/cro... File trytond/doc/topics/cron.rst (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:16: The timezone used for the "hour" computation of occurences is the timezone On 2022/05/09 21:02:39, ced wrote: > for the schedule computation comes from the ``TZ`` environment variable. Done. https://codereview.tryton.org/423091003/diff/411131003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:17: of the `TZ` environment variable. On 2022/05/09 21:02:38, ced wrote: > Use double ticks and run the check of the doc. Done. https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/__init... File trytond/trytond/__init__.py (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/__init... trytond/trytond/__init__.py:9: from trytond.config import config On 2022/05/09 21:02:39, ced wrote: > I do not like to import config in the __init__. > I see no problem to store the timezone as global variable. Done. https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/tests/... File trytond/trytond/tests/test_ir.py (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:389: class IrCronTestCase(ModuleTestCase): On 2022/05/09 21:02:39, ced wrote: > I still do not understand why testing twice the module. Done. https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/tools/... File trytond/trytond/tools/__init__.py (right): https://codereview.tryton.org/423091003/diff/411131003/trytond/trytond/tools/... trytond/trytond/tools/__init__.py:10: from dateutil.tz import gettz as ZoneInfo On 2022/05/09 21:02:39, ced wrote: > I think it will be better to have a time submodule which provide more feature > like the UTC and server timezone. > > It will also good in the future to provide the result of > https://docs.python.org/3/library/zoneinfo.html#zoneinfo.available_timezones Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
Missing tools documentation. https://codereview.tryton.org/423091003/diff/415101003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/CHANGELOG#newc... trytond/CHANGELOG:2: * Store local timezone setting in config no more valid https://codereview.tryton.org/423091003/diff/415101003/trytond/doc/topics/cro... File trytond/doc/topics/cron.rst (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:18: I do not see why there should be a new paragraph. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/__init... File trytond/trytond/__init__.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/__init... trytond/trytond/__init__.py:12: SERVER_TZKEY = os.environ.get('TZ', 'UTC') Why not just name it: TIMEZONE https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/config.py File trytond/trytond/config.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/config... trytond/trytond/config.py:52: self.server_tz = 'UTC' should be removed https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:105: return {c.id: tz_key for c in crons} For me it is simpler to use an instance method. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:189: now.astimezone(SERVER_TZ)) for me it is better that API is by default UTC. So the conversion should be don in compute_next_call https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... File trytond/trytond/tests/test_ir.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:397: activate_module(['ir'], lang='en') no need to set lang. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:407: trytond.ir.cron.SERVER_TZ = ZoneInfo('Canada/Eastern') trytond.ir.cron is not imported. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:411: "Test using a timezone far from UTC" Test compute next call on different timezone https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:438: "Test setting next call on an impossible hour" Test compute next call during summer daylight saving time https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:466: "Test setting next call at the 'same' hour but after DST occured" Test compute next call during winter daylight saving time https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... File trytond/trytond/tools/__init__.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/__init__.py:15: from .timezone import SERVER_TZ, TIMEZONES, UTC, ZoneInfo I think it is better to not include in base namespace. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:1: import trytond I think it is better to avoid circular import. Indeed I think simplest way would be to use an environment variable. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:2: I would add an __all__ to define clearly what is public. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:5: from zoneinfo import ZoneInfo I think it will be cleaner to do: ZoneInfo = zoneinfo.ZoneInfo https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:6: _tz_list_available = True I would just use zoneinfo as testing name. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:18: SERVER_TZ = ZoneInfo(trytond.SERVER_TZKEY) I would just name it: TIMEZONE https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:20: TIMEZONES = available_timezones() I think it will be better to be lazy on the computation. The cache could be managed by the available_timezones function.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/__init... File trytond/trytond/__init__.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/__init... trytond/trytond/__init__.py:12: SERVER_TZKEY = os.environ.get('TZ', 'UTC') I think it should be: os.environ.setdefault('TRYTOND_TIMEZONE', os.environ.get('TZ', 'UTC'))
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:14: return sorted(zoneinfo.available_timezones()) I think we should fallback to pytz and make it a requirements for Python version that does not have zoneinfo.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/415101003/trytond/CHANGELOG File trytond/CHANGELOG (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/CHANGELOG#newc... trytond/CHANGELOG:2: * Store local timezone setting in config On 2022/05/11 21:55:50, ced wrote: > no more valid Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/doc/topics/cro... File trytond/doc/topics/cron.rst (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:18: On 2022/05/11 21:55:50, ced wrote: > I do not see why there should be a new paragraph. Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/config.py File trytond/trytond/config.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/config... trytond/trytond/config.py:52: self.server_tz = 'UTC' On 2022/05/11 21:55:50, ced wrote: > should be removed Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:105: return {c.id: tz_key for c in crons} On 2022/05/11 21:55:50, ced wrote: > For me it is simpler to use an instance method. The class method is not that difficult and a bit more efficient. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:189: now.astimezone(SERVER_TZ)) On 2022/05/11 21:55:50, ced wrote: > for me it is better that API is by default UTC. > So the conversion should be don in compute_next_call Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... File trytond/trytond/tests/test_ir.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:397: activate_module(['ir'], lang='en') On 2022/05/11 21:55:51, ced wrote: > no need to set lang. Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:407: trytond.ir.cron.SERVER_TZ = ZoneInfo('Canada/Eastern') On 2022/05/11 21:55:51, ced wrote: > trytond.ir.cron is not imported. Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:411: "Test using a timezone far from UTC" On 2022/05/11 21:55:51, ced wrote: > Test compute next call on different timezone Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:438: "Test setting next call on an impossible hour" On 2022/05/11 21:55:51, ced wrote: > Test compute next call during summer daylight saving time Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:466: "Test setting next call at the 'same' hour but after DST occured" On 2022/05/11 21:55:51, ced wrote: > Test compute next call during winter daylight saving time Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... File trytond/trytond/tools/__init__.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/__init__.py:15: from .timezone import SERVER_TZ, TIMEZONES, UTC, ZoneInfo On 2022/05/11 21:55:51, ced wrote: > I think it is better to not include in base namespace. Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:5: from zoneinfo import ZoneInfo On 2022/05/11 21:55:51, ced wrote: > I think it will be cleaner to do: > > ZoneInfo = zoneinfo.ZoneInfo Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:6: _tz_list_available = True On 2022/05/11 21:55:51, ced wrote: > I would just use zoneinfo as testing name. Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:14: return sorted(zoneinfo.available_timezones()) On 2022/05/12 06:53:44, ced wrote: > I think we should fallback to pytz and make it a requirements for Python version > that does not have zoneinfo. Done. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:18: SERVER_TZ = ZoneInfo(trytond.SERVER_TZKEY) On 2022/05/11 21:55:51, ced wrote: > I would just name it: TIMEZONE It's too generic to my liking maybe TRYTOND_ZONEINFO as it should be imported as "from trytond.tools import timezone" it would result in timezone.TRYTOND_ZONEINFO in the code that need it. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:20: TIMEZONES = available_timezones() On 2022/05/11 21:55:51, ced wrote: > I think it will be better to be lazy on the computation. > The cache could be managed by the available_timezones function. Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:105: return {c.id: tz_key for c in crons} On 2022/05/14 21:50:24, nicoe wrote: > On 2022/05/11 21:55:50, ced wrote: > > For me it is simpler to use an instance method. > > The class method is not that difficult and a bit more efficient. For me we must use class method getter when there is an algorithmic advantage for it. Otherwise there is no point for supporting instance method. https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/415101003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:18: SERVER_TZ = ZoneInfo(trytond.SERVER_TZKEY) On 2022/05/14 21:50:25, nicoe wrote: > On 2022/05/11 21:55:51, ced wrote: > > I would just name it: TIMEZONE > > It's too generic to my liking maybe TRYTOND_ZONEINFO as it should be imported as > "from trytond.tools import timezone" it would result in > timezone.TRYTOND_ZONEINFO in the code that need it. I do not see the point of prefixing something in trytond namespace with TRYTOND_. How about: LOCAL https://codereview.tryton.org/423091003/diff/433111003/trytond/doc/topics/cro... File trytond/doc/topics/cron.rst (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:18: If none of the variables is set it defaults to UTC. For me the setup of timezone should not be documented here but in the tools section of timezone. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:70: timezone = fields.Function(fields.Char("Timezone"), 'get_timezone') This shadow the timezone import. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:157: now = datetime.datetime.now(tz=timezone.UTC) I do not think it is needed. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/tests/... File trytond/trytond/tests/test_ir.py (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/tests/... trytond/trytond/tests/test_ir.py:408: def test_using_non_utc_tz(self): test_scheduling_on_non_utc https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/tools/... File trytond/trytond/tools/__init__.py (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/tools/... trytond/trytond/tools/__init__.py:57: ] not for this patch. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:11: zoneinfo = False Usually we set to None the module that can not be imported. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:14: ALL_ZONES = [] Probably better to initialize to None so we can catch the case of no timezone are available. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:14: ALL_ZONES = [] I think it still good to make it private even if there is an __all__. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:25: return ALL_ZONES Should not be returning a copy to be sure that no code is messing with the cache.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/433111003/trytond/doc/topics/cro... File trytond/doc/topics/cron.rst (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:18: If none of the variables is set it defaults to UTC. On 2022/05/14 22:31:20, ced wrote: > For me the setup of timezone should not be documented here but in the tools > section of timezone. Done. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:70: timezone = fields.Function(fields.Char("Timezone"), 'get_timezone') On 2022/05/14 22:31:20, ced wrote: > This shadow the timezone import. Indeed but timezone is not used anywhere in the class declaration. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:157: now = datetime.datetime.now(tz=timezone.UTC) On 2022/05/14 22:31:20, ced wrote: > I do not think it is needed. It would be strange how could python apply "astimezone" on something that is not in a timezone?
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:70: timezone = fields.Function(fields.Char("Timezone"), 'get_timezone') On 2022/05/15 21:19:51, nicoe wrote: > On 2022/05/14 22:31:20, ced wrote: > > This shadow the timezone import. > > Indeed but timezone is not used anywhere in the class declaration. But in the get_timezone. I do not understand how it can work. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:157: now = datetime.datetime.now(tz=timezone.UTC) On 2022/05/15 21:19:51, nicoe wrote: > On 2022/05/14 22:31:20, ced wrote: > > I do not think it is needed. > > It would be strange how could python apply "astimezone" on something that is not > in a timezone? Indeed but then this should be done in compute_next_call to make the API consistent. https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/ref/tools/... File trytond/doc/ref/tools/timezone.rst (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:15: .. attritbute:: UTC attribute Please run the sphinx checker https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:23: The environment variables ``TRYTOND_TZ``, ``TZ`` are tested in this order to Better to use present instead of past for documentation. https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/topics/cro... File trytond/doc/topics/cron.rst (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:17: ``TRYTOND_TZ`` environment variable. Indeed it should refer to the timezone.SERVER https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:28: SERVER = ZoneInfo(os.environ['TRYTOND_TZ']) I would use a UTC fallback in case.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:70: timezone = fields.Function(fields.Char("Timezone"), 'get_timezone') On 2022/05/16 06:29:39, ced wrote: > On 2022/05/15 21:19:51, nicoe wrote: > > On 2022/05/14 22:31:20, ced wrote: > > > This shadow the timezone import. > > > > Indeed but timezone is not used anywhere in the class declaration. > > But in the get_timezone. I do not understand how it can work. classes do not define a scope (which is kind of strange because functions do). Anyway I change the import. https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:157: now = datetime.datetime.now(tz=timezone.UTC) On 2022/05/16 06:29:39, ced wrote: > On 2022/05/15 21:19:51, nicoe wrote: > > On 2022/05/14 22:31:20, ced wrote: > > > I do not think it is needed. > > > > It would be strange how could python apply "astimezone" on something that is > not > > in a timezone? > > Indeed but then this should be done in compute_next_call to make the API > consistent. I don't understand. What should be done in compute_next_call? now is used to find the crons and is the value that should be sent to compute_next_call in order not to miss any. https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/ref/tools/... File trytond/doc/ref/tools/timezone.rst (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:15: .. attritbute:: UTC On 2022/05/16 06:29:39, ced wrote: > attribute > > Please run the sphinx checker Done. https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:23: The environment variables ``TRYTOND_TZ``, ``TZ`` are tested in this order to On 2022/05/16 06:29:39, ced wrote: > Better to use present instead of past for documentation. It's not a past, it's the passive voice. And I find it better than the active form that is in the new review as the active form has an emphasis on Tryton, which I think is irrelevent (it should be on the variables). https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/topics/cro... File trytond/doc/topics/cron.rst (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/doc/topics/cro... trytond/doc/topics/cron.rst:17: ``TRYTOND_TZ`` environment variable. On 2022/05/16 06:29:39, ced wrote: > Indeed it should refer to the timezone.SERVER Done. https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:28: SERVER = ZoneInfo(os.environ['TRYTOND_TZ']) On 2022/05/16 06:29:39, ced wrote: > I would use a UTC fallback in case. It's useless since the code in trytond/__init__.py already does that.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... File trytond/trytond/ir/cron.py (right): https://codereview.tryton.org/423091003/diff/433111003/trytond/trytond/ir/cro... trytond/trytond/ir/cron.py:157: now = datetime.datetime.now(tz=timezone.UTC) On 2022/05/16 09:53:51, nicoe wrote: > On 2022/05/16 06:29:39, ced wrote: > > On 2022/05/15 21:19:51, nicoe wrote: > > > On 2022/05/14 22:31:20, ced wrote: > > > > I do not think it is needed. > > > > > > It would be strange how could python apply "astimezone" on something that is > > not > > > in a timezone? > > > > Indeed but then this should be done in compute_next_call to make the API > > consistent. > > I don't understand. > What should be done in compute_next_call? > now is used to find the crons and is the value that should be sent to > compute_next_call in order not to miss any. compute_next_call should accept a now without timezone and consider that it is always in UTC like in all Tryton. https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:28: SERVER = ZoneInfo(os.environ['TRYTOND_TZ']) On 2022/05/16 09:53:52, nicoe wrote: > On 2022/05/16 06:29:39, ced wrote: > > I would use a UTC fallback in case. > > It's useless since the code in trytond/__init__.py already does that. Except if Python fails to do that as Python doc suggests.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:28: SERVER = ZoneInfo(os.environ['TRYTOND_TZ']) On 2022/05/16 10:14:10, ced wrote: > On 2022/05/16 09:53:52, nicoe wrote: > > On 2022/05/16 06:29:39, ced wrote: > > > I would use a UTC fallback in case. > > > > It's useless since the code in trytond/__init__.py already does that. > > Except if Python fails to do that as Python doc suggests. I fail to see where the doc says that.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:28: SERVER = ZoneInfo(os.environ['TRYTOND_TZ']) On 2022/05/16 11:32:59, nicoe wrote: > On 2022/05/16 10:14:10, ced wrote: > > On 2022/05/16 09:53:52, nicoe wrote: > > > On 2022/05/16 06:29:39, ced wrote: > > > > I would use a UTC fallback in case. > > > > > > It's useless since the code in trytond/__init__.py already does that. > > > > Except if Python fails to do that as Python doc suggests. > > I fail to see where the doc says that. OK it seems that changing os.environ should not be a problem. Any way for me it is better to not fail here by having a fallback. Because there is no guarantee that this environ will always be set like any environ. https://codereview.tryton.org/423091003/diff/427191003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/427191003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:28: SERVER = ZoneInfo(os.environ['TRYTOND_TZ']) I think we must catch invalid zone info error, put a logging and set it to UTC.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:28: SERVER = ZoneInfo(os.environ['TRYTOND_TZ']) On 2022/05/16 11:49:49, ced wrote: > On 2022/05/16 11:32:59, nicoe wrote: > > On 2022/05/16 10:14:10, ced wrote: > > > On 2022/05/16 09:53:52, nicoe wrote: > > > > On 2022/05/16 06:29:39, ced wrote: > > > > > I would use a UTC fallback in case. > > > > > > > > It's useless since the code in trytond/__init__.py already does that. > > > > > > Except if Python fails to do that as Python doc suggests. > > > > I fail to see where the doc says that. > > OK it seems that changing os.environ should not be a problem. > Any way for me it is better to not fail here by having a fallback. Because there > is no guarantee that this environ will always be set like any environ. I don't understand as the guarantee is in the code of trytond/__init__.py
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:28: SERVER = ZoneInfo(os.environ['TRYTOND_TZ']) On 2022/05/16 13:58:08, nicoe wrote: > On 2022/05/16 11:49:49, ced wrote: > > On 2022/05/16 11:32:59, nicoe wrote: > > > On 2022/05/16 10:14:10, ced wrote: > > > > On 2022/05/16 09:53:52, nicoe wrote: > > > > > On 2022/05/16 06:29:39, ced wrote: > > > > > > I would use a UTC fallback in case. > > > > > > > > > > It's useless since the code in trytond/__init__.py already does that. > > > > > > > > Except if Python fails to do that as Python doc suggests. > > > > > > I fail to see where the doc says that. > > > > OK it seems that changing os.environ should not be a problem. > > Any way for me it is better to not fail here by having a fallback. Because > there > > is no guarantee that this environ will always be set like any environ. > > I don't understand as the guarantee is in the code of trytond/__init__.py Tryton is modular so we do not control all the code. Any way we must catch invalid zone. https://codereview.tryton.org/423091003/diff/435151003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/435151003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:37: "Timezone not found: %s, using UTC", os.environ['TRYTOND_TZ']) should not fail if TRYTOND_TZ is no in environ. https://codereview.tryton.org/423091003/diff/435151003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:37: "Timezone not found: %s, using UTC", os.environ['TRYTOND_TZ']) I would say: Timezone %r not found, fallback to UTC https://codereview.tryton.org/423091003/diff/435151003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:39: else: Why not combine both case into a single code like: try: SERVER = ZoneInfo(...) if not SERVER: raise ValueError except (KeyError, ValueError): logger.error(...) SERVER = UTC This way it works for all cases even if TRYTOND_TZ is no more in the environ.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/423231003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:28: SERVER = ZoneInfo(os.environ['TRYTOND_TZ']) On 2022/05/16 14:17:30, ced wrote: > On 2022/05/16 13:58:08, nicoe wrote: > > On 2022/05/16 11:49:49, ced wrote: > > > On 2022/05/16 11:32:59, nicoe wrote: > > > > On 2022/05/16 10:14:10, ced wrote: > > > > > On 2022/05/16 09:53:52, nicoe wrote: > > > > > > On 2022/05/16 06:29:39, ced wrote: > > > > > > > I would use a UTC fallback in case. > > > > > > > > > > > > It's useless since the code in trytond/__init__.py already does that. > > > > > > > > > > Except if Python fails to do that as Python doc suggests. > > > > > > > > I fail to see where the doc says that. > > > > > > OK it seems that changing os.environ should not be a problem. > > > Any way for me it is better to not fail here by having a fallback. Because > > there > > > is no guarantee that this environ will always be set like any environ. > > > > I don't understand as the guarantee is in the code of trytond/__init__.py > > Tryton is modular so we do not control all the code. With this reasoning there no guarantee over anything anywhere in the code ;). > Any way we must catch invalid zone. It's done in the latest patch https://codereview.tryton.org/423091003/diff/435151003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/435151003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:39: else: On 2022/05/16 14:17:30, ced wrote: > Why not combine both case into a single code like: > > try: > SERVER = ZoneInfo(...) > if not SERVER: > raise ValueError > except (KeyError, ValueError): > logger.error(...) > SERVER = UTC > > This way it works for all cases even if TRYTOND_TZ is no more in the environ. KeyError and ValueError are broad errors while we know that the key is set and the error that we want to catch. Hence doing so could catch too much things and hide another problem. But maybe the code can be made more compact.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:39: except zoneinfo.ZoneInfoNotFoundError: Will fail when zoneinfo is None. Really my proposal with KeyError is much simpler and neither catching too much. https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:43: SERVER = ZoneInfo('UTC') This part of the code is still duplicated.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:39: except zoneinfo.ZoneInfoNotFoundError: On 2022/05/16 16:26:11, ced wrote: > Will fail when zoneinfo is None. > > Really my proposal with KeyError is much simpler and neither catching too much. You don't know that it's not catching too much. But even if it's not the case currently the code of zoneinfo, tzdata or dateutil might change in the future. https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:43: SERVER = ZoneInfo('UTC') On 2022/05/16 16:26:11, ced wrote: > This part of the code is still duplicated. Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:39: except zoneinfo.ZoneInfoNotFoundError: On 2022/05/16 19:23:57, nicoe wrote: > On 2022/05/16 16:26:11, ced wrote: > > Will fail when zoneinfo is None. > > > > Really my proposal with KeyError is much simpler and neither catching too > much. > > You don't know that it's not catching too much. But even if it's not the case > currently the code of zoneinfo, tzdata or dateutil might change in the future. First the KeyError is documented in zoneinfo. Second if change happens we will know as there will be a logging. But any way if you care that much, you should add a test for invalid timezone. https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:38: if not zoneinfo and not SERVER: why testing zoneinfo? https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:40: except ZoneInfoNotFoundError: should catch KeyError if there is no TRYTOND_TZ in environ. https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:43: os.environ['TRYTOND_TZ']) must not fail. https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:44: SERVER = ZoneInfo('UTC') What is the point to make another instance?
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:38: if not zoneinfo and not SERVER: On 2022/05/16 20:40:25, ced wrote: > why testing zoneinfo? Because it's only when zoneinfo is None that SERVER can be None https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:40: except ZoneInfoNotFoundError: On 2022/05/16 20:40:24, ced wrote: > should catch KeyError if there is no TRYTOND_TZ in environ. There will be TRYTOND_TZ in the environ, it's guaranteed by the code in trytond/__init__.py https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:43: os.environ['TRYTOND_TZ']) On 2022/05/16 20:40:24, ced wrote: > must not fail. It won't. https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:44: SERVER = ZoneInfo('UTC') On 2022/05/16 20:40:24, ced wrote: > What is the point to make another instance? Done.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... File trytond/doc/ref/tools/timezone.rst (right): https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:7: .. class:: ZoneInfo(key) key is a strange name for me zoneid describe better as it is how IANA call it. https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:13: Get a list of all the valid IANA keys available. Return a sorted list of all available IANA zone ID. https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:17: A UTC ZoneInfo object. instance not object. https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:17: A UTC ZoneInfo object. I would use: The UTC ZoneInfo instance. https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:38: if not zoneinfo and not SERVER: On 2022/05/16 21:40:54, nicoe wrote: > On 2022/05/16 20:40:25, ced wrote: > > why testing zoneinfo? > > Because it's only when zoneinfo is None that SERVER can be None But it is pointless. The exception must be raise any time SERVER is None. https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:40: except ZoneInfoNotFoundError: On 2022/05/16 21:40:54, nicoe wrote: > On 2022/05/16 20:40:24, ced wrote: > > should catch KeyError if there is no TRYTOND_TZ in environ. > > There will be TRYTOND_TZ in the environ, it's guaranteed by the code in > trytond/__init__.py No it is not. It is a global variable that can be manipulated by any body.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:39: except zoneinfo.ZoneInfoNotFoundError: On 2022/05/16 20:40:24, ced wrote: > On 2022/05/16 19:23:57, nicoe wrote: > > On 2022/05/16 16:26:11, ced wrote: > > > Will fail when zoneinfo is None. > > > > > > Really my proposal with KeyError is much simpler and neither catching too > > much. > > > > You don't know that it's not catching too much. But even if it's not the case > > currently the code of zoneinfo, tzdata or dateutil might change in the future. > > First the KeyError is documented in zoneinfo. And? It's not because the error is a subclass of KeyError that a KeyError couldn't happen somewhere else. > Second if change happens we will know as there will be a logging. Isn't it better to fail than to work with a bad value (and logging)? > But any way if you care that much, you should add a test for invalid timezone. If you want.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/423091003
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/431131003/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:39: except zoneinfo.ZoneInfoNotFoundError: On 2022/05/17 07:40:18, nicoe wrote: > On 2022/05/16 20:40:24, ced wrote: > > On 2022/05/16 19:23:57, nicoe wrote: > > > On 2022/05/16 16:26:11, ced wrote: > > > > Will fail when zoneinfo is None. > > > > > > > > Really my proposal with KeyError is much simpler and neither catching too > > > much. > > > > > > You don't know that it's not catching too much. But even if it's not the > case > > > currently the code of zoneinfo, tzdata or dateutil might change in the > future. > > > > First the KeyError is documented in zoneinfo. > > And? > It's not because the error is a subclass of KeyError that a KeyError couldn't > happen somewhere else. > > > Second if change happens we will know as there will be a logging. > > Isn't it better to fail than to work with a bad value (and logging)? For me having the right timezone is not critical to the running server. Indeed I prefer to have a running server with UTC (as now) than no server. https://codereview.tryton.org/423091003/diff/411171003/trytond/trytond/tests/... File trytond/trytond/tests/test_tools.py (right): https://codereview.tryton.org/423091003/diff/411171003/trytond/trytond/tests/... trytond/trytond/tests/test_tools.py:419: self.assertEqual(zi.key, 'UTC') does not work with pytz
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... File trytond/doc/ref/tools/timezone.rst (right): https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:7: .. class:: ZoneInfo(key) On 2022/05/17 06:59:21, ced wrote: > key is a strange name for me zoneid describe better as it is how IANA call it. key is the name python use in their documentation. https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:13: Get a list of all the valid IANA keys available. On 2022/05/17 06:59:21, ced wrote: > Return a sorted list of all available IANA zone ID. Done. https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:17: A UTC ZoneInfo object. On 2022/05/17 06:59:21, ced wrote: > I would use: The UTC ZoneInfo instance. Done. https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:40: except ZoneInfoNotFoundError: On 2022/05/17 06:59:21, ced wrote: > On 2022/05/16 21:40:54, nicoe wrote: > > On 2022/05/16 20:40:24, ced wrote: > > > should catch KeyError if there is no TRYTOND_TZ in environ. > > > > There will be TRYTOND_TZ in the environ, it's guaranteed by the code in > > trytond/__init__.py > > No it is not. It is a global variable that can be manipulated by any body. Just like any other object can be manipulated so you can not rely on any assumption. If there is no reason to catch a KeyError when doing a pool.get, there is no reason to catch it here.
Sign in to reply to this message.
https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... File trytond/doc/ref/tools/timezone.rst (right): https://codereview.tryton.org/423091003/diff/413181004/trytond/doc/ref/tools/... trytond/doc/ref/tools/timezone.rst:7: .. class:: ZoneInfo(key) On 2022/05/17 08:19:40, nicoe wrote: > On 2022/05/17 06:59:21, ced wrote: > > key is a strange name for me zoneid describe better as it is how IANA call it. > > key is the name python use in their documentation. Python may have choose a not so great name. Also we make a tailored version so better to use name that suit us. https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... File trytond/trytond/tools/timezone.py (right): https://codereview.tryton.org/423091003/diff/413181004/trytond/trytond/tools/... trytond/trytond/tools/timezone.py:40: except ZoneInfoNotFoundError: On 2022/05/17 08:19:41, nicoe wrote: > On 2022/05/17 06:59:21, ced wrote: > > On 2022/05/16 21:40:54, nicoe wrote: > > > On 2022/05/16 20:40:24, ced wrote: > > > > should catch KeyError if there is no TRYTOND_TZ in environ. > > > > > > There will be TRYTOND_TZ in the environ, it's guaranteed by the code in > > > trytond/__init__.py > > > > No it is not. It is a global variable that can be manipulated by any body. > > Just like any other object can be manipulated so you can not rely on any > assumption. > > If there is no reason to catch a KeyError when doing a pool.get, there is no > reason to catch it here. It is a startup code that should not fail, in contrary to where pool.get is used inside a global catch all exception.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
