Remove remaining django remnants #5468
Conversation
sphuber
force-pushed
the
fix/remove-django-remnants
branch
from
f74988e
to
6e04fc2
Compare
sphuber
force-pushed
the
fix/remove-django-remnants
branch
from
6e04fc2
to
15fdae9
Compare
aiida/common/timezone.py
Outdated
| if value is None: | ||
| return None | ||
| return value.isoformat() | ||
| return value.isoformat() if value is not None else value |
There was a problem hiding this comment.
I know that this worked like this before, but since we are changing the behavior of the other methods I think it is a good moment to ask: is this really a good idea?
I mean, since the only purpose of this method is to operate on value, do we really want to accept and propagate None values? Is there a reason why this could be useful instead of just making errors slightly more difficult to catch? Is there a specific reason why is useful to ignore the None?
There was a problem hiding this comment.
A second question (that perhaps renders the previous one unnecessary) is: are we just keeping these methods for backwards compatibility? I see that later when we need to convert a isoformatted string to datetime we use datetime.fromisoformat directly instead of isoformat_to_datetime.
If we are not using these methods as a way to wrap the datetime tools to have a more modular dependency on them, then maybe we should deprecate these and take them out on the next iteration?
There was a problem hiding this comment.
Yeah, I think I agree. Especially since this is simply a method on the datetime object itself, no point to have a warpper function. Will remove this. Idem for isoformat_to_datetime.
aiida/common/timezone.py
Outdated
| if value is None: | ||
| return None | ||
| return parser.parse(value) | ||
| return datetime.fromisoformat(value) if value is not None else value |
There was a problem hiding this comment.
Idem the inverse method, we can discuss there and just leave this comment as reminder to also consider it here.
This was only used in `aiida.common.timezone.isoformat_to_datetime` and
in the `aiida.restapi.common.utils` module to parse a datetime object
from a string in ISO format. Since Python 3.7 this functionality is
provided in the `datetime` standard library:
https://docs.python.org/3/library/datetime.html#datetime.datetime.fromisoformat
The only relevant difference between the two implementations is that the
standard library is intentionally strict and will raise for invalid
formats whereas the `python-dateutil` will be lenient and try to guess
the intention in case of invalid formats. For both cases, we prefer not
to guess and potentially guess wrong, so the change is appropriate.
This function is used only to generate a password for the database user
when a new profile is setup through `verdi quicksetup`. Although this
password is not even critical, back in the early days when this was
added, the implementation for the method was taken from Django.
As of Python 3.6 though, there exists a much simpler solution that is
based on the `secrets` built in library. See the following for details:
https://docs.python.org/3/library/secrets.html#recipes-and-best-practices
The `make_aware` method was actually bugged if the passed `timezone` was not a `pytz.tzinfo` object but a standard `datetime.tzinfo`. The latter does not have the `localize` method. The implementation checked for this but outside the conditional still called the method, which would raise an `AttributeError`. Instead of fixing the small bug, the entire implementation is simplified significantly. The implementation was borrowed from Django way in the beginning of this project, but as of Python 3.6 `datetime.astimezone` provides all the needed functionality as it will automatically retrieve the timezone of the operating system when no timezone is specified. This also means that we can now drop the `tzlocal` dependency as its only use was determining the system's timezone. The `get_current_timezone` method that wrapped it is dropped. The function now no longer excepts if a datetime object is passed that is already aware, it will simply have the timezone updated. The `localtime` also was taken from Django originally and is also simplified. It also drops the `timezone` argument because with it, it is identical to `make_aware` rendering it obsolete. It should not allow specifying a `timezone` but always use the timezone of the OS. Similar to `make_aware`, this now also no longer excepts when an already aware object is passed. With these changes, the code no longer includes code taken from Django and so its license can be dropped from `open_source_licenses.txt`. Invocations of `timezone.delta` don't need to explicitly pass `timezone.now` as second argument as that is already the default. The use of `pytz` is reduced to a minimum and only used when there is no equivalent functionality in the standard library. Instead of `pytz.utc` use `datetime.timezone.utc`. For convenience this is forwarded in `aiida.common.timezone`. The `pytz` library is now exclusively used to convert a timezone name in string form into a `timezone` object. This functionality is centralized in `aiida.common.timezone.timezone_from_name`. This is also only used in `aiida.schedulers.datastructures` to convert the serialized response from schedulers. If this were to be removed, the `pytz` dependency could be dropped. Finally, the `aiida.manage.configuration.settings.USE_TZ` settingm is dropped. It was a relic of Django's configuration. It was hard-coded to true, wasn't configurable and was only used in `aiida.common.timezone.now` to make the returned object aware of the system's timezone if the setting was set to True. This behavior is now hard-coded.
sphuber
force-pushed
the
fix/remove-django-remnants
branch
from
15fdae9
to
342f7e6
Compare
|
Thanks for the review @ramirezfranciscof . Addressed your comments |
There was a problem hiding this comment.
Great, thanks!
Yes, I was suggesting to deprecate the removed methods in case some plugin or script was relying on those, but I guess they were supposed to be fairly internal to AiiDA and not something to be used outside the code. We are not a timestamp managing library after all 😅.
I had already removed other ones, exactly for the reason that these were merely intended for internal use. |
Fixes, simplifies and cleans up legacy code that was once taken from Django that now has more succinct solutions using the standard library. See commit messages for details.