Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've expected more from attributes. I'm kind of on the fence regarding this one.
Looking back at our previous conversations regarding retries (#342 (comment), #355 (comment)) I feel like I was not explicit enough.
The choice of creating
ApiWrapperWithRetries
is due to the reason that we can't really proxy requests from one class to another. We have magic __call that is triggered when invoking inaccessible methods in an object context, but our methods are accessible, they're public. Making them private and relying on__call
would open another can of worms, I really dislike it as an option, the comment from 7 years old still stands strong. __invoke is called when a script tries to call an object as a function e.g.(new ApiWrapper('Hello')
, so not applicable.Then come the attributes themselves. Having
#[Retry]
before a method call for a programmer coming from Java would look like, oh, hey, great, this will retry. It will not. Attribute names and their arguments are resolved to a class and the arguments are passed to its constructor, when an instance of the attribute is requested through the Reflection API. So we still need some underlying class to work the attributes.P. S.: No, it's not possible to add an attribute to an interface.
P. P. S.: On
ApiWrapperWithRetries implements ApiWrapperInterface
vsApiWrapperWithRetries extends ApiWrapper
implements was chosen due to the fact that having#[Retry]
before a method could seem that the retry will happen. In the implements approach, a developer would be forced to updateApiWrapperWithRetries
, if it would extendApiWrapper
, the margin of error is greater, as a developer would most likely update onlyApiWrapper
and then some time later we would discover that no retries are in fact happening.