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

[DO NOT MERGE] Upgrade Pub/Sub samples. #63

Closed
wants to merge 1 commit into from

Conversation

jmdobry
Copy link
Member

@jmdobry jmdobry commented Sep 26, 2016

Compare to:

Copy link
Contributor

@beccasaurus beccasaurus 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 this Jason!

Changes required to match some Ruby idioms (eg. don't use globals), match Ruby sample snippet templates, and specs are needed before merge.

require "google/cloud"

gcloud = Google::Cloud.new
$pubsub_client = gcloud.pubsub
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use global variables

# For more information, see the README.md under /pubsub and the documentation at
# https://cloud.google.com/pubsub/docs.

require "google/cloud"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include gem requirement and client instantiation in snippets

See example of Ruby sample snippet template: create_dataset

gcloud = Google::Cloud.new
$pubsub_client = gcloud.pubsub

# [START pubsub_list_subscriptions]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not include method definition in Ruby sample snippet

See example of Ruby sample snippet template: create_dataset


# [START pubsub_list_topic_subscriptions]
def list_topic_subscriptions topic_name:
# References an existing topic, e.g. "my-topic"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ruby sample snippets use in-line variable definitions that document required variables.

Example of Ruby sample snippet template: create_dataset

# project_id = "Your Google Cloud project ID"
# dataset_id = "ID of the dataset to create"

require "google/cloud"

gcloud   = Google::Cloud.new project_id
bigquery = gcloud.bigquery

bigquery.create_dataset dataset_id

puts "Created dataset: #{dataset_id}"

This provides a good developer user experience allowing the developer to copy the whole snippet into IRB or an .rb file, replace the placeholder variable, and get up-and-running

# [START pubsub_list_topic_subscriptions]
def list_topic_subscriptions topic_name:
# References an existing topic, e.g. "my-topic"
topic = $pubsub_client.topic topic_name, skip_lookup: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use skip_lookup option

To get a topic using google-cloud-pubsub:

require "google/cloud"

gcloud = Google::Cloud.new
pubsub = gcloud.pubsub

topic = pubsub.topic "existing-topic"

# References an existing topic, e.g. "my-topic"
topic = $pubsub_client.topic topic_name, skip_lookup: true

project_id = ENV["GCLOUD_PROJECT"] || "YOUR_PROJECT_ID"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sample snippets should not contain conditional logic. This snippet should not include a reference to environment variables. The project ID should be stored in variable that the developer can edit.

Please use a project_id variable such as the following convention followed by Ruby sample snippets:

# project_id        = "Your Google Cloud project ID"
# topic_name        = "Name of topic to subscribe to, eg. my-topic"
# subscription_name = "Name of subscription to create, eg. my-subscription"

require "google/cloud"

gcloud = Google::Cloud.new
pubsub = gcloud.pubsub
topic  = pubsub.topic

subscription = topic.subscribe(
  subscription_name,
  # Set to an HTTPS endpoint of your choice. If necessary, register
  # (authorize) the domain on which the server is hosted.
  endpoint: "https://#{project_id}.appspot.com/push"
)

puts "Subscription #{subscription.name} created."

@jmdobry jmdobry changed the title Upgrade Pub/Sub samples. [DO NOT MERGE] Upgrade Pub/Sub samples. Sep 26, 2016
@jmdobry
Copy link
Member Author

jmdobry commented Sep 26, 2016

See also Proposals implemented by the 3 PRs:

@beccasaurus
Copy link
Contributor

Pub/Sub sample has been updated to use the latest google-cloud-pubsub library. Existing Pub/Sub code is continuing to be improved, as is.

This code does not match idioms of Ruby, Google Cloud Ruby library, or that of other samples in this repository. Also missing test coverage. Can be reopened pending fixes and offline consistency decision making.

@beccasaurus beccasaurus deleted the pubsub-sample-style branch August 3, 2017 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants