Skip to content
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

Check timestamp with tz #132

Merged
merged 18 commits into from
Nov 29, 2023
Merged

Check timestamp with tz #132

merged 18 commits into from
Nov 29, 2023

Conversation

rp280
Copy link
Collaborator

@rp280 rp280 commented Nov 17, 2023

No description provided.

@rp280 rp280 requested a review from redfrexx November 17, 2023 14:54
@redfrexx
Copy link
Member

So the ohsome API always returns a timestamp with timezone (i.e. the Z in the end). ohsome-py returned timestamp objects without timezone in the functions where format="%Y-%m-%dT%H:%M:%SZ". I would prefer always returning a timestamp object with UTC timezone for convenience so the python users knows right away that it is UTC. Other opinions?

…the response for a dataframe and geodataframe
…rames(geoemtry) and geodataframes(groupByBoundary) is with timezone(UTC). Than the format parameter for every timestamp in the function _as_geodataframe was changed to ISO8601
@rp280 rp280 force-pushed the check_timestamp_with_TZ branch from 1da1c7b to fd700b5 Compare November 17, 2023 15:13
@SlowMo24
Copy link
Collaborator

I favour timestamps without time-zone. I never had a benefit only nuisance. For example you can note time zones differently (e.g. z== +00:00, yet the ohsome api does not support all these notions. That is why we had to remove the Zin #127

As you say: "everything is in UTC" so there is no benefit in marking that fact explicitly.

…the response for a dataframe and geodataframe
…rames(geoemtry) and geodataframes(groupByBoundary) is with timezone(UTC). Than the format parameter for every timestamp in the function _as_geodataframe was changed to ISO8601
@SlowMo24 SlowMo24 force-pushed the check_timestamp_with_TZ branch from fd700b5 to 6a31938 Compare November 17, 2023 19:35
@redfrexx
Copy link
Member

redfrexx commented Nov 20, 2023

True, I forgot about the 3.10 issue. Then let's have the timestamp without time zones. @rp280 can you adapt the code accordingly?

… is returning the same timestamp string without the character Z. Also the code was adjusted to remove the character z in the timestamp string
…s removed and a new test to check the contributions.count.density.groupByBoundary.post output was written
@rp280
Copy link
Collaborator Author

rp280 commented Nov 22, 2023

A Test for Issue #121 was also created in this Pull Request

@SlowMo24 SlowMo24 linked an issue Nov 23, 2023 that may be closed by this pull request
Copy link
Collaborator

@SlowMo24 SlowMo24 left a comment

Choose a reason for hiding this comment

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

Nice progress. Please update the CHANGELOG and state explicitely that we are deviating from the ohsome api here

ohsome/clients.py Outdated Show resolved Hide resolved
ohsome/response.py Outdated Show resolved Hide resolved
ohsome/response.py Outdated Show resolved Hide resolved
ohsome/test/test_response.py Outdated Show resolved Hide resolved
@SlowMo24 SlowMo24 self-requested a review November 29, 2023 14:27
@SlowMo24 SlowMo24 merged commit 927dd15 into master Nov 29, 2023
4 checks passed
@SlowMo24 SlowMo24 deleted the check_timestamp_with_TZ branch November 29, 2023 14:32
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.

different timestamp formats
3 participants