Move subscription methods up a level

[stephenplusplus]

Fixes #605

• Logic
• Unit tests
• System tests
  • Old functionality
  • ⚠️ New functionality - Move subscription methods up a level #618 (comment)
• Docs

[ryanseys]

Different than what I had in mind.

I was thinking more like:

// topic is in options
pubsub.subscribe('my-sub', { topic: pubsub.topic('some-topic') }, callback);

Thinking now, pubsub.subscribe('sub-name', topicObj, options, callback) makes more sense because its not an option in this case BUT a topic name is specified by its project and topic name like so: 'projects/project-id/topics/topic-name' so with your string version here, that wouldn't be the best unless that assumes that the topic is in the same project. It would be better to pass in a whole topic object.

Here's a partial attempt I made a while ago: https://github.com/ryanseys/gcloud-node/commit/78c4cfe6b06a7dcc1d44e226b00dbbc51a31d096 don't let it confuse you because it's not complete but something to gander at.

[stephenplusplus]

BUT a topic name is specified by its project and topic name like so: 'projects/project-id/topics/topic-name

I'm just getting the topic name (topic-name), then if the user gives a project-id, that project-id is used to make the full topic name, otherwise, use what's on the PubSub instance.

Quick API comparison :)

// ---
// This PR:
// ---
var pubsub = gcloud.pubsub({ projectId: 'grape-spaceship-123' });

// Each of these next two lines do the same thing:
pubsub.topic('some-topic').subscribe('my-sub', callback);
pubsub.subscribe('some-topic', 'new-sub-name', callback);

// Subscribe to a topic from another project.
pubsub.subscribe('another-topic', 'another-new-sub-name', { projectId: 'squishy-butter-334' }, callback);

// ---
// Your Idea:
// ---
var pubsub = gcloud.pubsub({ projectId: 'grape-spaceship-123' });

// Each of these next two lines do the same thing:
pubsub.topic('some-topic').subscribe('my-sub', callback);
pubsub.subscribe('new-sub-name', pubsub.topic('some-topic'), callback);

// Subscribe to a topic from another project.
var anotherPubsub = gcloud.pubsub({ projectId: 'squishy-butter-334' });

pubsub.subscribe('another-new-sub-name', anotherPubsub.topic('another-topic'), callback);

The main difference is requiring a Topic object vs. using explicit string params. I like both. Halp.

[jgeewax]

Couldn't we take bits of both?

var pubsub1 = gcloud.pubsub({ projectId: 'project1' });

// These all do the same thing
pubsub1.topic('some-topic').subscribe('my-sub', callback); // Most common pattern
pubsub1.subscribe('new-sub-name', 'some-topic', callback);  // Uncommon, but just a name implies same project
pubsub1.subscribe('new-sub-name', pubsub1.topic('some-topic'), callback); // Uncommon, most explicit.

// So then, if we want to subscribe to a topic in another project,
// the only way to do that is to create a topic object that knows it's project.
var pubsub2 = gcloud.pubsub({ projectId: 'project2' });
pubsub2.subscribe('new-sub-name', pubsub1.topic('some-topic'), callback);

[stephenplusplus]

I'm cool with that. Now just one smaller detail; shouldn't topic name come before subscription?

pubsub.subscribe('new-sub-name', 'some-topic', callback);
// vs.
pubsub.subscribe('some-topic', 'new-sub-name', callback);

I like the latter, just to follow the standard hierarchy "project -> topic -> subscription".

Just for completeness, here's the full sig:

pubsub.subscribe( ( Topic('topic-name') || 'topic-name' ), 'sub-name', [{ autoAck: true, ... }], function(err, subscription, apiResponse) {});

[jgeewax]

SGTM. Since topic is absolutely required, it should definitely be first.

[ryanseys]

Since topic is absolutely required, it should definitely be first.

Aren't both topic and sub-name required? Either order makes sense to me, so whatever you like :)

[jgeewax]

Yea, both required -- topic being higher up in the hierarchy seems like it should come first though. Sorry, didn't articulate that piece too. :P

[ryanseys]

Cool SGTM!

[stephenplusplus]

PTAL.

[stephenplusplus]

While Travis takes a year to run this build...

I'm not sure how I can write a system test for this functionality, since we only have access to one project. Unit testing covers all of the changes pretty well, since it was mostly just relocating code. To support subscribing from another project, all we're really doing is passing a different topic string to the API. The unit tests cover that as well. Is that enough?

[ryanseys]

I think this should be good. I can go ahead with a code review if you're ready?

[stephenplusplus]

Yes, please!

[stephenplusplus]

Merge time!