-
Notifications
You must be signed in to change notification settings - Fork 595
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
core: allow default method options for service objects #958
Conversation
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? |
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. |
I can't really! I think that is probably because of the |
core: allow default method options for service objects
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.
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'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.
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. |
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. |
- [ ] 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
This makes a small, but helpful, change to how methods are inherited from a ServiceObject.
Before:
After:
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 ageneration
property on the query string of a request.