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

Clear up makeRequest_ naming conventions #847

Closed
stephenplusplus opened this issue Sep 2, 2015 · 2 comments
Closed

Clear up makeRequest_ naming conventions #847

stephenplusplus opened this issue Sep 2, 2015 · 2 comments
Assignees
Labels

Comments

@stephenplusplus
Copy link
Contributor

We follow a few different patterns throughout the codebase, so I think we should try to sync up.

In some constructors, you might find this.makeRequest_ = storage.bucket.makeRequest.bind(storage.bucket). In other places, we define a makeRequest_ on the prototype. (props to @callmehiphop for first calling us out on this).

We should also clear up the naming conventions around making requests. Currently:

function Class() {
  this.makeAuthorizedRequest_ = util.makeAuthenticatedRequestFactory();
}

Class.prototype.makeReq_ = function(method, path, qs, body, callback) {
  var reqOpts = {
    method: method,
    uri: path,
    // ...
  };

  this.makeAuthorizedRequest_(reqOpts);
};

I think we should go through and have only one style; Class.prototype.request instead of constructor-initialized functions.

function Class() {
  this.authClient = util.getAuthClient();
}

Class.prototype.request = function(reqOpts, callback) {
  util.request(this.authClient, reqOpts, callback);
};

It should make it easier to follow along with, and it should be possible.

Also note using (reqOpts, callback) over (method, path, query, body, callback). It's pretty non-JS to have multiple arguments. We should just use the standard request options format, with reqOpts.uri, reqOpts.method, reqOpts.qs, reqOpts.json, etc.

LMKWYT!

@callmehiphop
Copy link
Contributor

👍

I'm all for this!

@stephenplusplus
Copy link
Contributor Author

Fixed by #904.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants