-
Notifications
You must be signed in to change notification settings - Fork 36
Conversation
Here is the summary of possible violations 😱 There are 32 possible violations for not having product prefix.
The end of the violation section. All the stuff below is FYI purposes only. Here is the summary of changes. You are about to add 32 region tags.
You are about to delete 2 region tags.
This comment is generated by snippet-bot.
|
Apologies for a large PR, but samples are quite repetitive: just different variations of the Data API reporting queries. |
Please let me know if splitting a PR would help a review, though. |
…data into beta_samples
…data into beta_samples
samples/snippets/get_metadata.py
Outdated
property, including custom fields.""" | ||
client = BetaAnalyticsDataClient() | ||
|
||
# [START analyticsdata_get_metadata_by_property_id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regions tags should show enough the user can copy/paste them to run, including imports - see "Region Tags" in the authoring guide.
It's also generally preferred to have 1 snippet per file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same everywhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kurtis, we do follow this advice for quickstart samples. However, other snippets are included in the API usage guides that demonstrate the usage of various API features inside one document (e.g. "Advanced API Features Guide", "Migration Guide"). It is therefore useful to include just the relevant part of the snippet to illustrate the feature. If a user wants to see the complete example, they can follow a link to GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example of the guide we will need to include the samples into:
https://developers.devsite.corp.google.com/analytics/devguides/reporting/data/v1/basics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I grouped certain samples together in one file since they demonstrate the same concept (e.g. run_report_with_filters.py
), just with different queries. Creating a separate file for each sample looks a bit redundant here, though we may consider separating these in the future. The more use cases we demonstrate, the easier it will be for the users to adopt the API...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should do it for each sample. The goal is that samples should be "copy-paste-runnable" - most users want to be able to copy a sample, paste it into their own environment, and run it immediately with only minor changes. This is what the sample format is optimizing for.
It may seem redundant but having each sample isolated into its own file makes it easier to ensure only used imports are included and that each snippet make sense without additional context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this makes sense. I moved each sample into a separate file and moved region tags around to include the entire sample.
samples/snippets/run_batch_report.py
Outdated
Dimension(name="city"), | ||
], | ||
metrics=[Metric(name="activeUsers")], | ||
date_ranges=[DateRange(start_date="7daysAgo", end_date="today")], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a link to some documentation around what format these should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would good if anywhere there is a "magic string" or value that the user need to customize themselves there is a comment reflecting that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced magic strings with absolute values in order not to distract the attention from the main point of the sample: batch requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we have the guides published, I'll update the description for each sample with a link to the relevant portion of the guide, which should also address the magic strings issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like there are still an awful lot of values that make no sense to me as someone trying to learn the API. Is there no reference documentation we can refer them to before the guide is published?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a link to the reference doc for every sample. Might replace this with a link to the corresponding guide document once it is published.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops - hit the wrong button. See the comments in the last review.
samples/snippets/get_metadata.py
Outdated
property, including custom fields.""" | ||
client = BetaAnalyticsDataClient() | ||
|
||
# [START analyticsdata_get_metadata_by_property_id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should do it for each sample. The goal is that samples should be "copy-paste-runnable" - most users want to be able to copy a sample, paste it into their own environment, and run it immediately with only minor changes. This is what the sample format is optimizing for.
It may seem redundant but having each sample isolated into its own file makes it easier to ensure only used imports are included and that each snippet make sense without additional context.
property="properties/" + str(property_id), | ||
dimensions=[Dimension(name="browser")], | ||
metrics=[Metric(name="activeUsers")], | ||
date_ranges=[DateRange(start_date="7daysAgo", end_date="yesterday")], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Missed a date range update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a link to the date range reference documentation instead.
|
||
def print_get_metadata_response(response): | ||
"""Prints results of the getMetadata call.""" | ||
# [START analyticsdata_print_get_metadata_response] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Do you still want this region tag here if the wider one includes this section?
|
||
def print_run_pivot_report_response(response): | ||
"""Prints results of a runPivotReport call.""" | ||
# [START analyticsdata_print_run_pivot_report_response] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Same w/ these region tags
🤖 I have created a release \*beep\* \*boop\* --- ### [0.5.1](https://github.com/googleapis/python-analytics-data/compare/v0.5.0...v0.5.1) (2021-05-28) ### Bug Fixes * **deps:** require google-api-core>=1.22.2 ([675ae9f](https://github.com/googleapis/python-analytics-data/commit/675ae9fb45bc4ea1adbbba1a302f04daf6368fbf)) ### Documentation * add sample code for Data API v1 ([#57](https://github.com/googleapis/python-analytics-data/issues/57)) ([a1e63c5](https://github.com/googleapis/python-analytics-data/commit/a1e63c56f5fa5835c528724c9d861c18cb34d6ad)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕