-
Notifications
You must be signed in to change notification settings - Fork 894
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
Resource API: constructors suffice for the Create API #191
Resource API: constructors suffice for the Create API #191
Conversation
As many programming languages are object-oriented, a constructor can satisfy the requirements of a Create function. Calling that out in the spec would be helpful to clarify.
Please sign the CLA. |
I just signed it, thanks for the reminder! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after signing the CLA, but a better solution might be to follow #165 and describe the behavior instead of the implementation. I.e. not say either "static" or "constructor".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -42,7 +42,7 @@ labels"](../semantic-conventions.md) that have prescribed semantic meanings. | |||
### Create | |||
|
|||
Creates a new `Resource` out of the collection of labels. This is a static | |||
method. | |||
method. For object-oriented languages, a constructor is also valid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a benefit for APIs to use static factory methods instead of ctor. See this article about java (also I think applies to other object-oriented languages) https://javarevisited.blogspot.com/2017/02/5-difference-between-constructor-and-factory-method-in-java.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link! I think that does provide great arguments for why a factory method works well.
What's the best way to add this insight to the spec? should we explicitly call out some of the advantage / disadvantages, or defer to a larger document discouraging constructors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't enforce factory methods usage - recommending them should be fine though.
(I get the advantages of factory methods, but I'd prefer to offer some flexibility here, honestly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not saying that we should enforce that, but I want everyone to understand the tradeoffs when they make a language specific decision. May be good to say that:
"we recommend factory method for these reasons a, b, c, but a direct constructor can be also used."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argument can be a generic comment that applies to all the ctor/static factory method (methods to create object in general), and in this specific way I would just say:
API MUST allow users to create a new Resource
from a collection of labels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"we recommend factory method for these reasons a, b, c, but a direct constructor can be also used."
This would be fine, yes ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think given the focus on capabilities, it might be more productive to have a PR around using a capabilities tone, although I plan on incorporating all the feedback from here.
I'll close this PR and start another one up around adhering to #165.
A focus of the specification on the capabilities that should be afforded rather than strict requirements around implementation enable better flexibility to ensure libraries are compatible with the idioms of their corresponding languages. This commit incorporates feedback discussed in open-telemetry#191.
A focus of the specification on the capabilities that should be afforded rather than strict requirements around implementation enable better flexibility to ensure libraries are compatible with the idioms of their corresponding languages. This commit incorporates feedback discussed in open-telemetry#191.
A focus of the specification on the capabilities that should be afforded rather than strict requirements around implementation enable better flexibility to ensure libraries are compatible with the idioms of their corresponding languages. This commit incorporates feedback discussed in #191.
…metry#196) A focus of the specification on the capabilities that should be afforded rather than strict requirements around implementation enable better flexibility to ensure libraries are compatible with the idioms of their corresponding languages. This commit incorporates feedback discussed in open-telemetry#191.
…metry#196) A focus of the specification on the capabilities that should be afforded rather than strict requirements around implementation enable better flexibility to ensure libraries are compatible with the idioms of their corresponding languages. This commit incorporates feedback discussed in open-telemetry#191.
As many programming languages are object-oriented, a constructor can satisfy
the requirements of a Create function. Calling that out in the spec would
be helpful to clarify.
I spoke to @carlosalberto briefly about this, and he clarified. I figured it would be great to clarify.