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

core: allow default method options for service objects #958

Merged
merged 1 commit into from
Nov 24, 2015

Conversation

stephenplusplus
Copy link
Contributor

This makes a small, but helpful, change to how methods are inherited from a ServiceObject.

Before:

I'm a new class that needs to inherit these methods:

  • delete
  • getMetadata
  • setMetadata

After:

I'm a new class that needs to inherit these methods:

  • delete
  • getMetadata
  • setMetadata, but use PUT instead of PATCH

I've had to work around the last scenario, using PUT over the default PATCH, and similar things before. The old way to fix this was re-implementing the same method, changing a minor detail.

I will implement this behavior in future PRs. It will help out resource manager and also simplify the File class which conditionally sets a generation property on the query string of a request.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 24, 2015
@callmehiphop
Copy link
Contributor

My only complaint with this is that we build out the request config every time a method is used, even though it never changes. Why not do it at instantiation time and/or cache it?

@stephenplusplus
Copy link
Contributor Author

Why

Just for code cleanliness, as opposed to micro-optimization. If you can think of a way to do it that isn't weird/bloated/out of place, I'm all for it.

@callmehiphop
Copy link
Contributor

If you can think of a way to do it that isn't weird/bloated/out of place

I can't really! I think that is probably because of the ServiceObject architecture though. We've created an interesting problem for ourselves where we try and have a base class that acts as a catch all for upstream APIs, but they don't all behave exactly the same.

callmehiphop added a commit that referenced this pull request Nov 24, 2015
core: allow default method options for service objects
@callmehiphop callmehiphop merged commit 6449524 into googleapis:master Nov 24, 2015
@stephenplusplus
Copy link
Contributor Author

I think that is probably because of the ServiceObject architecture though

I think it's just because of JavaScript. Regardless of the container the code is in, caching something adds complexity and bloat. Caches have to be stored somewhere.

where we try and have a base class that acts as a catch all for upstream APIs, but they don't all behave exactly the same.

That's why it's a base class. We have to generalize the 90% use case, and not leave the 10% with nothing. Configuration is how to do it, but I can't argue that "this is the best code ever." If you see a more clear Service/ServiceObject architecture (one that doesn't allow configuration?), lay out some samples in a new issue.

@callmehiphop
Copy link
Contributor

I think it's just because of JavaScript. Regardless of the container the code is in, caching something adds complexity and bloat. Caches have to be stored somewhere.

I'm not saying caching is the ultimate solution, I also wouldn't blame a language since I think this discussion is directly related to our service/service object architecture.

That's why it's a base class. We have to generalize the 90% use case, and not leave the 10% with nothing. Configuration is how to do it, but I can't argue that "this is the best code ever." If you see a more clear Service/ServiceObject architecture (one that doesn't allow configuration?), lay out some samples in a new issue.

I understand this, but I'm wondering if we generalized a little too much. I don't really have any great ideas like I said earlier, I'm just a little worried about how much configuration we'll need as our API coverage increases.

@stephenplusplus
Copy link
Contributor Author

I can understand the core concern with not wanting to have over-confugurarion. I hope after this, we are in good standing to really not need any. The point of this class was only to not reproduce code. Now that we're there, we can start to refactor and probably improve that code. But for now, I'm glad we were able to generalize enough to affect almost all of the methods, since that was goal number 1.

sofisl pushed a commit that referenced this pull request Nov 11, 2022
- [ ] Regenerate this pull request now.

Use gapic-generator-typescript v2.14.5.

PiperOrigin-RevId: 450616838

Source-Link: googleapis/googleapis@7a47b72

Source-Link: googleapis/googleapis-gen@42cc633
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNDJjYzYzMzFiYWUwYjk5ZjYxYjhlMDFhZTE1YjA1MjExNzE2YzRmOSJ9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants