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

teams and services analytics endpoints #312

Merged
merged 4 commits into from
Mar 18, 2021
Merged

teams and services analytics endpoints #312

merged 4 commits into from
Mar 18, 2021

Conversation

@theckman theckman added this to the v1.4.0 milestone Mar 12, 2021
analytics.go Outdated
@@ -9,6 +9,7 @@ type AnalyticsRequest struct {
AggregateUnit string `json:"aggregate_unit,omitempty"`
TimeZone string `json:"time_zone,omitempty"`
}

type AnalyticsResponse struct {
Data []AnalyticsData `json:"data,omitempty"`
AnalyticsFilter *AnalyticsFilter `json:"filters,omitempty"`
Copy link
Collaborator

@theckman theckman Mar 13, 2021

Choose a reason for hiding this comment

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

Apologies for asking this of you since this wasn't your change, but your PR made me look at things again and I am now realizing I missed this in the original PR. Would you rename this field in your PR to just be Filters *AnalyticsFilter. We've not released this yet, and so I'd rather we get this done right versus letting it bleed out once we do release v1.4.0.

Copy link
Author

Choose a reason for hiding this comment

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

AnalyticsFilter has been changed to Filters 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you may have misread my comment, I'm sorry. Was only wanting the field name updated, not the type too. The type made sense because it's the filters for the Analytics stuff, but AnalyticsResponse.AnalyticsFilters stutters a little bit, while AnalyticsResponse.Filters is easier to communicate.

Copy link
Author

Choose a reason for hiding this comment

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

I did misread your comment. Apologies 😅 I went ahead and changed that in the new commit. Please let me know if you see anything else 👍

analytics.go Outdated Show resolved Hide resolved
analytics.go Show resolved Hide resolved
analytics.go Show resolved Hide resolved
Copy link
Collaborator

@theckman theckman left a comment

Choose a reason for hiding this comment

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

Apologies for not being more clear in my comment asking for the field name change. When you have a moment could you back the type name changes out, but keep the field name changes? Sorry for the trouble.

@theckman theckman merged commit a9bc8eb into PagerDuty:master Mar 18, 2021
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.

2 participants