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.

Sign up for GitHub

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

Jump to bottom

Add osm-timestamps to indicators #101

Merged
merged 18 commits into from
Aug 16, 2021
Merged

Add osm-timestamps to indicators #101

merged 18 commits into from
Aug 16, 2021

Conversation

Gigaszi
Copy link
Contributor

@Gigaszi Gigaszi commented Jul 21, 2021 •
edited by matthiasschaub

Description

Add osm-timestamps to indicators and add timezone to oqt-timestamp.

Corresponding issue

Closes #1

Checklist

  • I have updated my branch to main (e.g. through git rebase main)
  • My code follows the style guide and was checked with pre-commit before committing
  • I have commented my code
  • I have added sufficient unit and integration tests
  • I have updated the CHANGELOG.md

@matthiasschaub
Copy link
Collaborator

Hey @Gigaszi,

I left some minor comments and suggestion about code readability, but otherwise this looks great. I changed to PR title and description slightly.

@matthiasschaub matthiasschaub changed the title Add osm-timestamps for indicator + tests for it. Add timezone for oqt… Add osm-timestamps to indicators Jul 23, 2021
matthiasschaub
matthiasschaub previously approved these changes Jul 23, 2021
View reviewed changes
matthiasschaub
matthiasschaub requested changes Jul 28, 2021
View reviewed changes
workers/ohsome_quality_analyst/indicators/mapping_saturation/indicator.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@joker234 joker234 mentioned this pull request Jul 29, 2021
Closed
5 tasks
matthiasschaub
matthiasschaub reviewed Aug 11, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@matthiasschaub
Copy link
Collaborator

matthiasschaub commented Aug 11, 2021 •
edited

Looks good to me!

Now we explicitly depend on dateutil (Not just via third-party library) we need to add it to our dependencies.

@Gigaszi please run poetry add dateutil and commit poetry.lock and pyproject.toml to this branch. Also update the PR description that new dependency was added. Thanks :)

matthiasschaub
matthiasschaub previously approved these changes Aug 12, 2021
View reviewed changes
joker234
joker234 requested changes Aug 13, 2021
View reviewed changes
CHANGELOG.md Outdated
Comment on lines 31 to 32
Add osm-timestamps to indicators ([#101])
- Add timezone to oqt-timestamp ([#101])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Add osm-timestamps to indicators ([#101])
- Add timezone to oqt-timestamp ([#101])
- Add osm-timestamp to indicators ([#101])
- Add timezone to oqt-timestamp ([#101])

workers/ohsome_quality_analyst/base/indicator.py Outdated
@@ -75,7 +75,8 @@ def __init__(
data_class=LayerDefinition, data=get_layer_definition(layer_name)
)
self.result: Result = Result(
timestamp_oqt=datetime.utcnow(),
# Aware UTC datetime object representing the current time.
Copy link
Member

Choose a reason for hiding this comment

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

Aware of what? Do you mean Timezone aware or UTC aware?

workers/tests/integrationtests/test_indicator_ghs_pop_comparison_buildings.py Outdated
Comment on lines 34 to 37
self.assertIsNotNone(self.indicator.result.timestamp_osm)
self.assertIsInstance(self.indicator.result.timestamp_osm, datetime)
self.assertIsNotNone(self.indicator.result.timestamp_oqt)
self.assertIsInstance(self.indicator.result.timestamp_oqt, datetime)
Copy link
Member

Choose a reason for hiding this comment

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

@matthiasschaub do you see a reason why we need the additional self.assertIsNotNone here? I think these checks are redundant. The same applies to the changes in the other tests.

@matthiasschaub matthiasschaub added this to the Release 0.5.0 milestone Aug 16, 2021
@Gigaszi Gigaszi requested a review from joker234 August 16, 2021 09:36
@Gigaszi Gigaszi merged commit c6e198c into main Aug 16, 2021
@Gigaszi Gigaszi deleted the timestamps branch August 16, 2021 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joker234 joker234 joker234 approved these changes

@matthiasschaub matthiasschaub matthiasschaub left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 0.5.0
Development

Successfully merging this pull request may close these issues.

Add OSM/data timestamp to indicator result object
3 participants
@Gigaszi @matthiasschaub @joker234