10000 Added list_timestamps by claezon · Pull Request #192 · vikinganalytics/mvg · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Added list_timestamps #192

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
Merged
merged 11 commits into from
Feb 3, 2023
Merged

Added list_timestamps #192

merged 11 commits into from
Feb 3, 2023

Conversation

claezon
Copy link
Contributor
@claezon claezon commented Jan 19, 2023 •
edited
Loading

Description

Added function to retrieve timestamps from vibium-cloud endpoint /sources/{source_id}/timestamps.
References vikinganalytics/vibium-cloud#690

New dependencies: (Added to requirements_*.txt)

Fixes # (potential issues that are fixed by this PR)

Type of Change

  • Bug fix (non-breaking change which fixes an issue -> bump patch)
  • New feature (non-breaking change which adds functionality -> bump minor version)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected -> bump major version)
  • Documentation Update
  • CI/CD workflows Update

Checklist

  • I have added tests that prove that my fix/feature works
  • Linters pass locally and I have followed PEP8 code style
  • New and existing tests pass locally
  • I have updated the documentation if needed
  • I have commented hard-to-understand areas in the code

Requirements

  • I have updated the MVG version

@codecov
Copy link
codecov bot commented Jan 26, 2023 •
edited
Loading

Codecov Report

Base: 72.14% // Head: 72.81% // Increases project coverage by +0.67% 🎉

Coverage data is based on head (581873f) compared to base (5ee949b).
Patch coverage: 90.90% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   72.14%   72.81%   +0.67%     
==========================================
  Files          11       12       +1     
  Lines         858      883      +25     
==========================================
+ Hits          619      643      +24     
- Misses        239      240       +1
Impacted Files Coverage Δ
mvg/utils/response_processing.py 82.60% <82.60%> (ø)
mvg/mvg.py 89.42% <100.00%> (+1.03%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@claezon claezon marked this pull request as ready for review January 26, 2023 10:20
@claezon claezon requested review from vnadhan and tuix January 26, 2023 10:20
vnadhan
vnadhan requested changes Jan 26, 2023
View reviewed changes
Copy link
Contributor
@vnadhan vnadhan left a comment

Choose a reason for hiding this comment

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

A few comments here, Rickard.. I will add comments iteratively if that is Ok.

@claezon claezon requested a review from vnadhan January 31, 2023 13:30
vnadhan
vnadhan reviewed Feb 1, 2023
View reviewed changes
vnadhan
vnadhan reviewed Feb 1, 2023
View reviewed changes
vnadhan
vnadhan reviewed Feb 1, 2023
View reviewed changes
vnadhan
vnadhan requested changes Feb 1, 2023
View reviewed changes
Copy link
Contributor
@vnadhan vnadhan left a comment

Choose a reason for hiding this comment

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

Thanks for working on the comments, Rickard. A few more comments from my side.

tuix
tuix previously requested changes Feb 2, 2023
View reviewed changes
Copy link
Contributor
@tuix tuix left a comment

Choose a reason for hiding this comment

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

great job, just a question regarding the return value of list_timestamps to be consistent with list_measurements

@claezon claezon requested review from tuix and vnadhan February 2, 2023 11:46
vnadhan
vnadhan previously approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor
@vnadhan vnadhan left a comment

Choose a reason for hiding this comment

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

Good work Rickard. Feel free to merge

8000
@vnadhan vnadhan force-pushed the timestamps_endpoint branch from 3556c6b to 8bddc52 Compare February 3, 2023 07:59
vnadhan
vnadhan previously approved these changes Feb 3, 2023
View reviewed changes
@vnadhan vnadhan self-requested a review February 3, 2023 09:10
@vnadhan vnadhan merged commit 99858bd into master Feb 3, 2023
@vnadhan vnadhan deleted the timestamps_endpoint branch February 3, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vnadhan vnadhan vnadhan approved these changes

@tuix tuix Awaiting requested review from tuix

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@claezon @tuix @vnadhan
0