Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

Modify NodeJS codegen to fit with gcloud-node design. #392

Merged
merged 4 commits into from
Aug 23, 2016

Conversation

jmuk
Copy link
Contributor

@jmuk jmuk commented Aug 18, 2016

  • The new style exports a function instead of the class. The
    exported function will take the auth context as a parameter
    and returns builder function.
  • Files will be under a sub directory with versioned name,
    so that it can be exposed as a sub package of the API package.
  • A few misc changes and style changes made to pass lint
    checker in gcloud.

@jmuk
Copy link
Contributor Author

jmuk commented Aug 18, 2016

The generated result is reviewed at googleapis/google-cloud-node#1476 and probably some details will change later.

jmuk added a commit to jmuk/packman that referenced this pull request Aug 18, 2016
- retire use_pbjs flag -- it's not used anymore, and it does not
  fit with the new pattern of Gapic code.
- update the grpc package template: this fits with the proto packages
  used by gcloud-node.
- update gax package template: this will work well with the new
  pattern of Gapic code, as you can see in googleapis/gapic-generator#392
  or googleapis/google-cloud-node#1476
- The new style exports a function instead of the class. The
  exported function will take the auth context as a parameter
  and returns builder function.
- Files will be under a sub directory with versioned name,
  so that it can be exposed as a sub package of the API package.
- A few misc changes and style changes made to pass lint
  checker in gcloud.
jmuk added a commit to jmuk/packman that referenced this pull request Aug 18, 2016
- retire use_pbjs flag -- it's not used anymore, and it does not
  fit with the new pattern of Gapic code.
- update the grpc package template: this fits with the proto packages
  used by gcloud-node.
- update gax package template: this will work well with the new
  pattern of Gapic code, as you can see in googleapis/gapic-generator#392
  or googleapis/google-cloud-node#1476
@jmuk
Copy link
Contributor Author

jmuk commented Aug 18, 2016

It seems the auto-generated code would be fine. @geigerj please take a look (@swcloud is added FYI)

@codecov-io
Copy link

codecov-io commented Aug 18, 2016

Current coverage is 81.40% (diff: 83.33%)

Merging #392 into master will increase coverage by 0.24%

@@             master       #392   diff @@
==========================================
  Files           183        185     +2   
  Lines          6904       6943    +39   
  Methods           0          0          
  Messages          0          0          
  Branches        900        905     +5   
==========================================
+ Hits           5603       5652    +49   
+ Misses         1026       1017     -9   
+ Partials        275        274     -1   

Powered by Codecov. Last update f71215b...c6f9f37

* The version of the calling service.
*/
function {@serviceName}(opts) {
function {@serviceName}(gaxGrpc, grpcClient, opts) {

This comment was marked as spam.

This comment was marked as spam.

- constructor parameters are not actually useful now. They are
  parameters to the builder function actually, so I moved there.
- build() is not a function, now it's a class. This way, JSDoc
  can generate documentations for its methods (like builder function).
- add an example clause in the constructor doc comments, so that
  documentation says the details of how to create an instance of
  the API client.
@jmuk
Copy link
Contributor Author

jmuk commented Aug 22, 2016

Modified the code a bit. Please take another look.

Here are the key points:
The API class is now not exposed and users are expected to create an instance through a builder function, like this.

var pubsubV1 = require('@google-cloud/pubsub').v1();
var publisher = pubsubV1.publisherApi();
publisher.createTopic(...);

So now, publisherApi (not a capitalized PublisherApi) is a function and it actually takes the all optional parameters. Thus,

  • define a Builder class, and let this builder function (like publisherApi) to be a method of this class. This way, JSDoc will pick up the builder function in the generated HTML.
  • move the documentation of parameters to the builder function. The parameters for the constructor is intentionally blank, because it's not expected to be built directly.
  • add an example clause in the constructor document of how to create an instance.

if (names.size() >= 3) {
return lowerUnderscoreToLowerCamel(
names.get(names.size() - 3) + "_" + names.get(names.size() - 2));
} else if (names.size() >= 2) {

This comment was marked as spam.

This comment was marked as spam.

@geigerj
Copy link
Contributor

geigerj commented Aug 22, 2016

Two nits, otherwise LGTM

@geigerj
Copy link
Contributor

geigerj commented Aug 22, 2016

LGTM if build passes

@jmuk jmuk merged commit 94de9f2 into googleapis:master Aug 23, 2016
jmuk added a commit to jmuk/packman that referenced this pull request Sep 7, 2016
- retire use_pbjs flag -- it's not used anymore, and it does not
  fit with the new pattern of Gapic code.
- update the grpc package template: this fits with the proto packages
  used by gcloud-node.
- update gax package template: this will work well with the new
  pattern of Gapic code, as you can see in googleapis/gapic-generator#392
  or googleapis/google-cloud-node#1476
jmuk added a commit to googleapis/packman that referenced this pull request Sep 8, 2016
- retire use_pbjs flag -- it's not used anymore, and it does not
  fit with the new pattern of Gapic code.
- update the grpc package template: this fits with the proto packages
  used by gcloud-node.
- update gax package template: this will work well with the new
  pattern of Gapic code, as you can see in googleapis/gapic-generator#392
  or googleapis/google-cloud-node#1476
- add 'env' parameter to dependency_out protoc
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants