update style-guide regarding @Metadata

[DartBot]

This issue was originally filed by [email protected]

---

Now that we have some reflective access to metadata (woot!), it might be a good time to update the style guide regarding best practices.

I've written up my thoughts about this here:
http://futureperfect.info/2013/06/20/a-case-for-metadata.html

The point that (I believe) needs to be addressed, is that current annotations such as @override or @observe don't mix well with annotations that use constant constructors, such as a fictional @ExpectError(StateError)

cheers,

[dgrove]

Set owner to @munificent.
Added Area-StyleGuide, Triaged labels.

[sigmundch]

Issue #14149 has been merged into this issue.

[sigmundch]

Marked this as blocking #14860.

[sigmundch]

Marked this as blocking #15159.

[sigmundch]

Thanks Ross for putting together those notes.

I mentioned this in a related bug, but I'll copy it here so we can have style discussions in a single place.

I think annotations are different enough to deserve different style rules.

In my opinion @­ALL_CAPS is not easy read, and I personally find @­foo more readable than @­Foo(). I'm open to have an exclusion in the style-guide saying that classes intended for annotations can be named differently, so you can user them as @­lowerCamelCase or @­lower_case_with_underscores.

I think this is ok because we rarely, if ever, use a class both for code and for annotations.

[peter-ahe-google]

My preference is to define lower case constants when they are intended for use as annotations, and keep the current conventions for class names. So:

const deprecated = const Deprecated();

@deprecated foo() {}

@deprecated() bar() {}

[DartBot]

This comment was originally written by @seaneagan

---

Siggi,

Thanks for starting this discussion!

Regarding readability, if issue #2 were fixed, what would you then find more readable:

@foo
@foo

For me, it's @­Foo because:

* that's what I'm used to with java/C#

• it reads similar to a type annotation (e.g. Comparable)

[DartBot]

This comment was originally written by @seaneagan

---

@6 (Peter): It seems unfortunate that every annotation would need to have a parallel named top-level constant, just so () can be omitted at the use site. That means:

* extra API surface
  - 2 ways to do the same thing (@­deprecated, @­Deprecated())
  - duplicate documentation.

• name shadowing due to an abundance of top-level variables (e.g. imported @target shadowed by local target constant, no error given when using @target)

issue #2 solves all this by allowing @­Deprecated

[peter-ahe-google]

You don't need to duplicate anything, I only used "deprecated" to illustrate my opinion on naming conventions.

If it wasn't too late, I'd argue that we should remove "deprecated".

[DartBot]

This comment was originally written by @seaneagan

---

Here is the style guide rule that I believe we need:

PREFER exposing constructor invocation annotations ( e.g. @­Foo() ) over top-level variable annotations ( e.g. @­foo ).

This provides a more consistent experience for annotation users. It also future proofs the annotation for adding optional parameters in the future ( e.g. @­Foo(bar: baz) ). There is also the possibility that the user will be able to omit the () in the future (see http://dartbug.com/13582), which would make it just as short ( e.g. @­Foo ) anyways.

GOOD:

    class Foo() {
      const Foo();
    }

    // ...

    @­Foo() method();

BAD:

    class _Foo() {
      const _Foo();
    }

    const foo = const _Foo();

    // ...

    @­foo method();

[DartBot]

This comment was originally written by [email protected]

---

I'm happy to see this conversation happening, and thankful for Sean's persistence :)

This appears to be a fairly subjective topic. For what it is worth, I am in favor of comment #­10. In the 5 months since I wrote this original issue, I have worked quite a bit with annotations in Dart. Probably the only notable thing I could say now is that I have gotten used to the way it is. I'm afraid we may end up with a 50/50 split of lowercase and Uppercase annotations, like we have with /// and /** */ doc comments, unless there is strong pressure to go one way or the other, as there seem to be strong preferences out there for both ways. Maybe that is okay.

[peter-ahe-google]

I strongly agree with the last sentence of comment #­11 :-)

[DartBot]

This comment was originally written by @seaneagan

---

I feel like the main (only ?) reason some folks prefer @­foo is they can't have @­Foo (issue #2), but maybe I'm wrong, and at least I would be much happier to use natural selection to resolve this if one of the selections wasn't artificially hobbled with the () tax :).

[munificent]

TL;DR at the bottom...

OK, so I scraped all of the packages in pub and did some digging to see how people are using annotations right now.

I found 832 usages of capitalized annotations. The most frequent ones were:

• @­Test() (386 usages)
• @­NgDirective() (99 usages)
• @­CustomTag() (85 usages)
• @­NgComponent() (24 usages)

I found 2737 usages of lowercase annotations. The most frequent were:

• @­override (2018 usages)
• @­observable (194 usages)
• @­deprecated (181 usages)
• @­published (154 usages)
• @­protected (51 usages)
• @­specification (40 usages)
• @­reflectable (40 usages)

This seems like lowercase is the clear winner. However, that's dominated by the three annotations in corelib: @­override, @­deprecated, and @­protected. If you omit those, there are 487 remaing usages of lowercase annotations.

If you look at the different kinds of annotations, we have:

• 74 different capitalized annotations
• 23 different lowercase annotations

I found several capitalized annotations without parameters (so presumably references to classes). I also found a few lowercase annotations with parameters, so lowercase class names like @­groupdoc('Multi groupdoc') and @­specification(name: "contains()").

Of the constructor annotations (i.e. ones with parameters), I found all manner of optional, positional, and named parameters. It's clear that people are using the flexibility of parameter lists to constructors. There are a number of cases where an empty parameter list is being used.

I even found a couple of named constructors like @­WithState.visible().

My intuition from this data is that there are a few annotations that feel like they are part of the language itself, almost like pseudo-reserved words. People seem to prefer those to be lowercase, like @­override and @­protected.

Some frameworks like polymer have also chosen lowercase, like @­observable and @­published.

But, outside of those, the wide majority of annotations are actually capitalized class constructor calls. Further, users are taking advantage of parameter lists in many cases there.

Given that, I feel that the style guide should not go out of its way to encourage people to make constants to hide their annotation classes behind. There's no need to do:

class Foo { const Foo(); }
const foo = const Foo();

When people seem perfectly comfortable doing @­Foo or @­Foo() and this gives you more flexibility going forward.

However, I don't think we should prohibit making constants for your annotations. For some cases where the annotation is really frequently used, people seem to like something visually lighter.

The only issue with that is that it conflicts with our ALL_CAPS guideline for constants. My belief is that the style guide should change that rule. We don't currently apply it consistently, and we've frequently found exceptions to it. I've never liked it and don't think it adds value.

Instead, I think the style guide should say that some constants are ALL_CAPS and others are lowerCamelCase and either style is acceptable.

TL;DR: I want feedback, but my current belief is that we should make these changes to the style guide:

1. For classes to be used in annotations say that they are named normally. This isn't strictly necessary, but it clarifies things.
2. Say annotations are normally just class constructors or class literals if you want to leave off the "()".
3. But for annotations that will be heavily used, it's OK to define a constant with a lowercase name and have users use that.
4. Constants can be lowerCamelCase or ALL_CAPS, with a slight preference for the former.

Thoughts?

[peter-ahe-google]

I like Bob's suggestion, but I'll note that the bit about class literals in annotations should be removed for now. Shortly before shipping 1.0, it was made illegal to use class literals in annotations because it was feared that Java programmers would be confused.

[DartBot]

This comment was originally written by @seaneagan

---

Wow, thanks Bob!

My analysis of the data:

@foo was never working in all (any?) of the implementations (and had flawed semantics in any that did), so that left annotation designers with a choice between @­foo and @­Foo(), and many didn't want to force their users to write the parentheses, so they went with @­foo. Further, that was the path taken by the core package:meta annotations (@­override, @­deprecated, @­proxy), which set a precedent, which was then followed by many. And that was then reinforced by the decision to move these annotations to dart:core.

I don't think the annotation usage data is very relevant here. The lowercase ones are almost all dart:core and polymer annotations, and the upper case are Angular or polymer ( not sure about @­Test() ). Users don't have a choice with the core annotations, and don't select their UI framework (or any other really) based on annotation style :).

Regarding the proposed rules:

1. Agree!
2. Agree!
3. Disagree on these points:

a) There should be a strong warning that choosing @­foo prevents ever adding optional arguments to the annotation.

b) It reinforces the false dilemma between @­foo ( see a) ) and @­Foo() (required parentheses. Instead we should just remove this dilemma by fixing issue #2 ( allowing @­Foo ) which gives the best of both worlds.

c) It gives annotation designers the impression that @­foo is the better choice if they want their annotation to be heavily used, and what annotation designer doesn't want that :).

1. Agree!

[sigmundch]

I don't entirely agree. I view the fact that an annotation is defined as a class or a constant as an implementation detail. Normally, symbols for annotations are used mostly as annotations and not both as annotations and expressions in code. So, how an annotation is implemented is hidden enough that you don't need to know whether it's one or the other. Syntactically we already have '@', so we have more freedom to choose what we think works best.

I believe the reason behind many naming conventions in the style guide is analogous. We suggest CamelCase for classes and camelCase for methods and variables to help distinguish from each other and improve readability when we use them. Because definitions for annotations are only intended to be used as an annotation, I think the guideline should focus mainly on what feels more natural as a user of an annotation, and should not be based solely on existing rules about what's used to implement them.

Maybe it helps if we frame the question as follows: suppose we had a way in the language to declare annotations with a special keyword. That is, we no longer use 'class' or constants for this, but something of this form:

  annotation myAnnotation(String mandatoryArg, {optionalNamedArg})

then, would that change our naming convention? What would our convention be?

I think Bob's comment above totally makes sense that aesthetically @­lowercase annotations make it feel like the annotation is more integrated with the language. So coming back to Sean's question in comment #­7 above, I might still prefer @­foo over @­Foo.

For example, I'd be perfectly happy with an annotation for written as @­lower_case, for example:

   annotation test_case(String description, {expectedResult})

   ...
   
   @­test_case('name should say "foo"')
   myNameIsFoo() { ... }

In today's implementation, that would mean 'test_case' is a class, but that should be be OK :-).

I totally agree though that we should fix issue #2 to get rid of the dilemma Sean mentions in #­16 (3b) above.

[DartBot]

This comment was originally written by @seaneagan

---

Siggi, agree that most of the time the annotation user doesn't care about
the implementation.

But, if users can expect a certain implementation, then this makes it
easier for them to find them in the documentation (e.g. under the classes
section). Ideally this problem could be solved with an @­Annotation
annotation (which could be placed on classes or top-level constants). This
annotation could be consumed by DartDoc, and possibly by the analyzer to
give warnings when you are using something in an annotation that is not
marked an an @­Annotation. But I wonder if it is too late for this solution.

Also, here are some exceptions where the implementation may be important:

1. There is at least one existing case where a declaration is used both as
   a metadata annotation, and in programming logic:

import 'dart:async';
@lazy
import 'package:lazy/lazy.dart';

const lazy = const DeferredLibrary('lazy');

Future lazyLoad() => lazy.load();

2. I also have a use case where I want to use the same class to
   declaratively annotate something:

@command(help: '...')
method();

or programmatically use it:

addCommand(method, new Command(help: '...'));

3. There is also the possibility that the module consuming an annotation
   may not be the same as the one defining the annotation. For example,
   database annotations like @­Table(), @­Column() etc. could be defined in some
   ORM package, and then consumed by various database adapter packages, which
   may need to understand the implementation of the annotation.

[DartBot]

This comment was originally written by @seaneagan

---

I am running into a problem in comment 18, 2) above, where my imperative Command class needs to have extra API surface beyond what is in my declarative @­Command annotation. I think for this use case a different naming convention (as suggested by Siggi) for annotations may be useful. Bob mentioned elsewhere that this problem was handled in C# by transforming Command used in an annotation to CommandAttribute. Now that dart is 1.0, I don't think we can do any such automatic transformations. So we need a different class for the annotation form, and to reference that directly at the annotation use site.

Assume the imperative class is FooBar. Here are some options for the annotation class:

a) @­FooBarMeta() // slightly redundant

b) @­fooBar() // likely to conflict with var fooBar = new FooBar()

c) @­FooBar_() // ugly

d) @­foo_bar() // common case of @­foo() has same issues as b) above.

e) Use same name, but put annotations in a separate library. User decides which to prefix, the annotation library or base library. Feels harder for annotation consumers to grok, and possibly error prone (if both classes have a const constructor):

@foobar()

or

@meta.FooBar()

Thoughts on these options or others?