-
Notifications
You must be signed in to change notification settings - Fork 1.7k
dart:core: replace @foo with @Foo() #14860
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
Comments
Added Area-Library, Library-Core, Triaged labels. |
CCing people that have more opinions on annotations. cc @lrhn. |
I agree completely. I think the proposed approach would be much better. (And it would be kind of cute to see 'deprecated' be marked as being deprecated.) |
This comment was originally written by @seaneagan Looks like we ran out of time on this one. IMO, there should still be a style guide rule (issue #4) to at least PREFER @Foo() over @foo, and this should be done quickly before a bunch of important packages which expose annotations start reaching 1.0 status. It could be seen as quite similar to num/int/double/bool violating the UpperCamelCase class names style guide rule. I filed issue #9 specifically for polymer/observe annotations. |
I like the idea in 13582 that you can omit '()' as it will make it easier to transition between no-arg and some args on some annotations. However, I personally prefer @foo over @Foo(). I understand this might be a matter of taste, but maybe we should try to agree on what we want the styleguide for annotations to be. I'd be more inclined to have a different style for annotations that makes them especially readable. For example, if a class is intended for annotations, maybe we want to use @lowerCamelCase or @lower_case_with_underscores rather than @UpperCamelCase. Just my 2 cents. |
Removed Type-Defect label. |
This issue was originally filed by @seaneagan
Currently in dart:core we have top-level variables for annotations:
@deprecated
@Proxy
@OverRide
But in order to allow specifying an expiration date, the Deprecated class is also public:
@deprecated('1/1/2014')
This means when I refactor between the 2 forms, I have to convert between 'd' and 'D', which is annoying. If the argument to the Deprecated constructor were made optional, then
deprecated
could be removed, since you could just do:@deprecated()
If we ever need to add an optional argument to @proxy, we would similarly need to add a Proxy class, but we couldn't add it to dart:core, because it would break existing apps, and putting it in some other library away from @proxy would be really strange. For example, maybe we would want to limit the interfaces implemented by the proxy, to those explicitly stated in the implements clause:
@Proxy(explicitInterfaces: true)
class Foo implements A, B {
// ...
noSuchMethod(Invocation invocation) {
}
}
... so that
new Foo().someBogusField
still yields warnings:Also, if this pattern of @foo + @Foo(bar) is introduced in dart:core, it will set a style precedent to be followed by other annotations. This will lead to a lot of unnecessary top-level variables, which lead to name conflicts, shadowing issues, and unnecessary API surface.
Suggestion is to instead use:
@deprecated()
@Proxy()
@OverRide()
And now with issue #7 fixed, there is the possibility of issue #2 getting fixed in the future, at which point these could be shortened to:
@deprecated
@Proxy
@OverRide
The text was updated successfully, but these errors were encountered: