Skip to content

[62132] Fix issue when converting offset-aware datetime objects to timestamp#154

Merged
mrashed-dev merged 8 commits into
mainfrom
mostafarashed/ch62132/github-issue-143-cannot-use-timezone-aware
Jun 11, 2021
Merged

[62132] Fix issue when converting offset-aware datetime objects to timestamp#154
mrashed-dev merged 8 commits into
mainfrom
mostafarashed/ch62132/github-issue-143-cannot-use-timezone-aware

Conversation

@mrashed-dev
Copy link
Copy Markdown
Contributor

Description

Wherever we accept a date as a filtering parameter, we usually convert it from a datetime object to a timestamp object. However the way we convert it does not take into consideration if the datetime is offset-aware or not (timezone), and thus would throw an error when it's attempted. Now we check if the datetime is context-aware, and if it is, we convert it properly to being an offset-naive object before converting it to a timestamp.

License

I confirm that this contribution is made under the terms of the MIT license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@shortcut-integration
Copy link
Copy Markdown

@mrashed-dev mrashed-dev linked an issue Jun 10, 2021 that may be closed by this pull request
`datetime.timezone` does not exist on Python < 3.3 so we have to use `pytz` to provide us a timezone example
@mrashed-dev mrashed-dev changed the title Fix issue when converting offset-aware datetime objects to timestamp [62132] Fix issue when converting offset-aware datetime objects to timestamp Jun 11, 2021
Comment thread nylas/utils.py
# return delta.total_seconds()

return delta.seconds + delta.days * 86400

Copy link
Copy Markdown

@jseller jseller Jun 11, 2021

Choose a reason for hiding this comment

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

maybe a good idea to use timedelta from the datetime package for adding or subtracting time values from datetime.

Also, datetime has a timestamp() method, so that could be just: return dt.timestamp()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing my attention to timedelta it helps clean the code a bit! Regarding datetime.timestamp() I initially used that as the solution, but unfortunately it's only available from Python 3.3 and up, and we still support Python 2.7 I believe so I had to scrap that idea.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok, this is good for py2, that timestamp() is in py3. datetime in python isn't the best package

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So I've heard :/

@mrashed-dev mrashed-dev requested a review from jseller June 11, 2021 19:00
Copy link
Copy Markdown

@jseller jseller left a comment

Choose a reason for hiding this comment

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

lgtm

@mrashed-dev mrashed-dev merged commit b2ef206 into main Jun 11, 2021
@mrashed-dev mrashed-dev deleted the mostafarashed/ch62132/github-issue-143-cannot-use-timezone-aware branch June 11, 2021 19:07
mrashed-dev added a commit that referenced this pull request Jul 21, 2021
New `nylas` v5.0.0 release bringing in the following additions:
* Add support for the Nylas Neural API (#163)
* Add `metadata` support (#152)
* Add new Room Resource fields  (#156)
* Add `Nylas-API-Version` header support (#157, #151)

as well as the following changes:
* Transitioned from `app_id` and `app_secret` naming to `client_id` and `client_secret` (#159, #86)
* Fix adding a tracking object to an existing `draft` (#153)
* Fix issue when converting offset-aware `datetime` objects to `timestamp` (#154, #143)
* Fix `limit` value in filter not being used when making `.all()` call (#155)
* Fix `from_` field set by attribute on draft ignored (#162, #160)
* Remove `bumpversion` from a required dependency to an extra dependency (#158, #144)
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.

Cannot use timezone aware datetimes in messages API

2 participants