Step one towards stable error messages with details:

Step one towards stable error messages with details:
 - switches polymer to use a messages type
 - autogenerates an HTML page from the message list.
 - use these messages on polymer transformers
 - moves build_logger to code_Transformers, so we can also use this in other packages.

Still pending to do more, for example, find a permanent location and URI scheme for messages
so we can show them on the command-line, improve the UI to tak advantage of clustering, etc.

But I think it's good enough to get it in and iterate afterwards

R=jakemac@google.com, kathyw@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39884

[Siggi Cherem (dart-lang), 2014-09-03 01:51:23]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2014-09-03 02:00:29]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2014-09-03 02:07:40]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2014-09-03 02:11:00]

sigmund@google.com changed reviewers:
+ jakemac@google.com, kathyw@google.com

[Siggi Cherem (dart-lang), 2014-09-03 02:11:01]

This CL has a blend of code and lots of text documenting warning messages.

Kathy - Could you take a look at each 'messages.dart' file containing text
explanations (there should be 3). I'll try tomorrow to host for you the HTML to
make it easier to review too.

Thanks!

[jakemac, 2014-09-03 17:20:42]

Looks good, mostly just small nits. LGTM

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/messages/build_logger.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/build_logger.dart:18: /// This logger records
all messages that were log and then forwards
s/log/logged

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/build_logger.dart:45: /// Records a message
at the fine level, [msg] can be a [String] or [Message].
Technically it doesn't have to be a String, it can be anything (the way its
implemented)

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/build_logger.dart:47: var snippet = msg is
Message ? msg.snippet : '$msg';
I don't think either of these variables are really necessary (same for other
functions):

msg = msg is Message ? msg : new Message(
    MessageId.NOT_SPECIFIED, '$msg');
_transform.logger.fine(message.snippet, asset: asset, span: span);
_logs.add(new BuildLogEntry(message, span, LogLevel.FINE.name));

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/build_logger.dart:89:
JSON.encode(_logs.toJson())));
I thought JSON.encode will automatically call toJson?

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/messages/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/messages.dart:34: toString() =>
'${package}_$id';
imo, a ':' would be better than an '_'

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/messages.dart:63: const Message(this.id,
this.snippet);
A named constructor or factory that takes no ID and defaults to NOT_SPECIFIED
might be nice to have, maybe Message.unknown/Message.notSpecified/Message.simple

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/messages.dart:67: String toString() =>
'${toJson()}';
I think something like '$id: $snippet' might be better visually than a map.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/messages.dart:93: /// Short description of
the error message
How does this differ from snippetTemplate and details?

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:10: const noAbsolutePaths = const
MessageTemplate(
These should really be all caps names since they are global constants
(throughout)

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/tool/create_...
File pkg/polymer/tool/create_message_details_page.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/tool/create_...
pkg/polymer/tool/create_message_details_page.dart:98: font-weight: 700;
it doesn't look like you are using both of these versions, just font-weight
normal (not sure which one of these that is actually defaulting to use?). Same
for roboto.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/tool/create_...
pkg/polymer/tool/create_message_details_page.dart:115: width: 80vw;
indent

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/tool/create_...
pkg/polymer/tool/create_message_details_page.dart:178: display: inline;
indent

[Kathy Walrath, 2014-09-03 18:56:36]

Here's a review of the first messages.dart file (code_transformers). Midway
through the review, I realized that the bulk of the second message should just
have its own page, so I'll create that.

More to come...

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:21: 'Invalid url to reach to another
package: %-url-%. Path '
Global change to text:
url -> URL

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:23: 'way to the %-prefix-% folder.
For example, try changing the url above '
is it clear what "above" means?

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:23: 'way to the %-prefix-% folder.
For example, try changing the url above '
Be consistent about using folder vs. directory. I believe that in the docs we
usually use "directory".

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:27: To reach an asset that belongs
to another package, we use `package:` URLs in
we use -> use

In general, avoid "we" in messages. It's unclear who "we" is, and usually not
necessary.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:28: Dart code, but in any other
language (like HTML or CSS) we use relative URLs.
we use -> use

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:30: These are the rules you must
follow to write URLs that refer to files in other
-> Follow these rules when writing...

This is way long, so I'll create a document that contains these rules. (You can
ignore the rest of the comments on this message!)

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:33: * if the file containing the
relative URL is an entrypoint under `web`, use
if -> If

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:36: * if the file containing the URL
is under `web`, but on a different directory
if -> If
on -> in

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:40: **Note**: this means that if you
have two entrypoints including this file,
this means that if you have two entrypoints including
->
If two entrypoints include

this file -> the same file that's under `web` (? "this file" didn't seem clear
enough)

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:44: * if the file containing the URL
lives under `lib`, walk up as many levels as
if -> If

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:48: The rules are easier to follow
if you know how the code is layed out for
layed -> laid

(global)

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:54: '- a1.html
Could you use | instead of '? You used | below (for c/web/c1), and it seemed
clearer to me.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:75: If your app is package `c`,
you'll notice that in your file system there is a
you'll notice that in your file system there is a packages folder generate in
the web folder
->
then `pub build` generates a packages folder under the web folder

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:94: `index.html` in Dartium, it will
infer that the package root is under
what is "it"? Also, avoid "there is" when possible. Finally, I found myself
getting a bit confused about whether this was really `pub build` output, since
no `build` directory was visible. This seemed like it might be just `pub get`
output.

Maybe:

Note that no `lib` directory is under the `packages` directory.
When Dartium loads `web/index.html`, Dartium infers that the package root
is under `web/packages/`.

I'm not sure "infers" is the right word here, but maybe it is. Maybe it's more
like this:

...Dartium loads libraries from package roots directly under `web/packages`.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:154: produce an error message like
this one, instead.
This is a lot of doc! How about I put it in a page, so we can have real figures
and it's more easily editable. It seems like these messages should give a hint
and then link to the doc page.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:161: 'Incomplete import to asset in
another package',
to -> of?

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:165: requiremnt, but for now we ask
that you use a canonical URL form for it.
requiremnt -> requirement

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:168: `packages/b/b.html`, you could
technically just write `../b/b.html`, but we
who is "we" really? polymer.dart? The polymer transformer? Maybe it doesn't
really matter?

Maybe:

For example, if `packages/a/a.html` needs to import `packages/b/b.html`,
you might expect a.html to import `../b/b.html`. Instead, it must import
`../../packages/b/b.html`.
See ...

[Kathy Walrath, 2014-09-03 19:36:06]

observe package's messages.dart

(I reviewed more than what was visible on the page, which I didn't do before.
Let me know if you want me to re-review the other messages.dart file(s) for
non-page-visible messages.)

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
File pkg/observe/lib/src/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:13: 'It should be placed on individual
fields.',
It should... fields.
->
Instead, annotate individual fields as @observable.
[GLOBAL]

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:14: '`@observable` on a library is not
supported.',
These messages have a slightly different format from the other messages. To make
these consistent, I'll take out the verb and the terminating ".", and (for
consistency within this group) make the object plural.

->
'`@observable` not supported on libraries',

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:21: '`@observable` on top-level fields is not
supported.',
->
'`@observable` not supported on top-level fields',

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:28: '`@observable` on classes is not
supported.',
->
'`@observable` not supported on classes',

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:34: 'Observable fields should be put in
observable objects.',
should be put in -> must be in
[GLOBAL]

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:35: '`@observable` on static fields is not
supported.',
->
'`@observable` not supported on static fields',

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:40: 'Observable fields should be put in an
observable '
an observable -> observable

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:41: 'objects. Please declare that this class
extends from '
Please declare -> Change this class to extend, include, or implement Observable.

[What does "include Observable" mean?]

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:44: '`@observable` must be used on a class
that implements `Observable`.',
->
'`@observable` field not in an `Observable` class',

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:51: must explicitly opt-in on each field you
wish to make observable.
I'm not crazy about having the same text for each message. I spent a little time
looking at the page to make sure they didn't differ. It might be better to have
the common message at the top, directly under "Messages from package observe".

If we keep the common message:

Only instance fields on `Observable` classes can be observable,
and you must explicitly annotate each observable field as `@observable`.

(That started as just changing "opt-in" to "opt in", but then I realized "opt
in" might not be clear. It has too many uses of the word "observable", but I
couldn't think of a clearer way to say it.)

[Kathy Walrath, 2014-09-03 21:26:56]

I'm starting to go a little cross-eyed here, but here's what I found in the 3rd
messages.dart file.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:28: * There are two different elements
defined with the same name.
There are two -> Two
defined with -> have [or "are defined that have" or "are defined with"]

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:29: * A single HTML file defining an
element, is imported from two different
, is imported from -> has been imported using

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:40: Starting with polymer 0.11.0 we
require that each file that uses the definition
0.11.0 we require that each -> 0.11.0, each

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:41: of polymer-element has to import it
either directly or transitively.
has to import -> must import

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:48: 'Please move the import out of this
<polymer-element>.',
Please move -> Move

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:51: HTML imports are expected on the top
of each document, outside of any
on the -> at the

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:54: build process will ignore imports
that appear to be in the wrong location.
will ignore -> ignores

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:59: 'To run a polymer application, you
need to call "initPolymer". You can '
"initPolymer" -> initPolymer()

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:66: We determined that your application
entry point didn't have any Dart script
->
Your application entrypoint doesn't have...

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:67: tags, and hence, it's missing some
initialization needed for polymer.dart.
->tags, so it's...

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:74: 'No script tags with experimental
bootstrap.',
Remove the "."

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:80: 'Single tag per document',
Most of these are phrased to describe the problem, not the solution. Maybe:

Multiple Dart script tags found

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:82: Dartium currently limits to have a
single script tag per document. Any
limits to have a single
->
allows only one

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:163: 'Script tags should have either a
URL or an inlined body.',
->
Script with both src and inline text

(be sure to remove the ".")

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:165: You have a script tag that includes
both a `src` attribute, and script text
attribute,...
->
attribute and inline script text.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:176: 'Incorrect instantiation: missing
base tag in instantiation.',
remove the "."

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:198: This is because browsers first
create the base element, and then upgrade them to
them -> it

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:201: doing the upgrade.
what is the base type that they need to create
->
which base type it must create

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:212: 'in declaration.',
remove the "."

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:225: or that the declaration needs to be
fixed and include the `extends` attribute,
or that ->
Another possibility is that

and include -> to include

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:293: 'Event handler bindings are method
names, not expressions.',
remove the "." and make it not a sentence. Maybe:

No expressions allowed in event handler bindings

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:303: 'Nested polymer element definitions
are not allowed.',
Nested polymer element definitions not allowed

(removing the verb and the ".")

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:306: `<polymer-element>` defined nested
within a `<polymer-element>`. If you have a
defined -> definition?

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:307: definition inside another, please
move the sencond definition out.
please move -> move
sencond -> second

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:309: It's possible that you see this
error if you have an HTML import within a
It's possible that you -> You might

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:310: polymer element. You should be able
to also move the import out of the element
to also -> to

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:316: 'Missing tag name of the custom
element. Please include an '
Please include -> Include

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:318: 'Polymer element definitions must
have a name.',
->
Polymer element definition without a name

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:329: 'at least one dash and can\'t be
any of the following names: '
dash -> dash (-)

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:331: 'Custom element names must have a
dash.',
->
Custom element name missing a dash

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:333: Custom element names must have a
dash and can\'t be any of the following
dash -> dash (`-`)

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:342: 'Failed to inline html import:
%-error-%',
html -> HTML

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:343: 'An error occurred while iniling an
import',
iniling -> inlining

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:352: 'An error occurred while iniling a
stylesheet.',
->
Error while inlining a stylesheet

(remove the verb and the ".", fix the spelling of inlining)

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:354: An error occurred while iniling a
stylesheet in the polymer build. This is often
GLOBAL:
iniling -> inlining

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:372: 'Not all browsers support bindings
to URL attributes',
This message seems more specific than the previous string indicates. They should
either both cover URLs or not be so specific to URLs. Maybe:

'Attribute missing "_" prefix'

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:374: Not all browsers support bindings
to URL attributes. Some browsers might
Delete the first sentence if it just repeats the message. Also, doesn't this
apply to _style and _value, as well as _src and _href?

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:393: 'The special underscore attribute
is only for bindings.',
->
Attribute has "_" prefix'

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:410: Sorry, you just run into a bug in
the polymer transformer code. Please file a
you just run -> you just ran
[GLOBAL]

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:411: bug at <http://dartbug.com/new>
including if possible some example code that can
including if possible some
->
including, if possible, some
[GLOBAL]

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:412: help us reproduce the issue so we
can investigate.
delete " so we can investigate"

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:432: '`@CustomTag` is only supported on
public classes.',
->
`@CustomTag` used on a private class

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:435: you need to register a custom
element whose implementation is a private class,
Maybe clarify that it's private because its name starts with a _ or its
library/package names starts with _? Otherwise, Java programmers will think they
have to add "public".

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:443: '`@initMethod` is only supported on
public functions.',
->
`@initMethod` used on a private function

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:458: 'Invalid argument in annotation',
->
Non-constant argument in annotation

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:460: We expected a constant argument in
an annotation, but the polymer build was not
->
The polymer transformer was not able to extract a constant value for the
annotation argument.
This can happen...

(getting rid of "we", plus "build" implies pub build to me, but it could be pub
serve, no? but maybe you really didn't mean the transformer?)

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:500: 'Private symbols cannot be used in
event handlers',
->
Private symbol in event handler

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:502: Currently private members can't be
used on event handlers. So you can't write:
on -> as? with? for?

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:513: 'Private symbols are not
supported',
->
Private symbol in a binding expression

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:524: 'A warning was found while parsing
the html document',
html -> HTML
[global within text]

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:526: We use a parser that implements the
HTML5 spec (`html5lib`). We report any
We use a
->
The polymer transformer uses a

We report -> This message reports

that were detected by our....
->
that the parser detects.

[Siggi Cherem (dart-lang), 2014-09-04 02:32:18]

oh wow! thanks so much for all your comments Kathy!

I think I have addressed all if not most of them. Let me know if you prefer
other ways to iterate on this (for example, I'm perfectly happy submitting and
letting you edit them later).

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/messages/build_logger.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/build_logger.dart:18: /// This logger records
all messages that were log and then forwards
On 2014/09/03 17:20:41, jakemac wrote:
> s/log/logged

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/build_logger.dart:45: /// Records a message
at the fine level, [msg] can be a [String] or [Message].
On 2014/09/03 17:20:41, jakemac wrote:
> Technically it doesn't have to be a String, it can be anything (the way its
> implemented)

Good point :), rephrased

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/build_logger.dart:47: var snippet = msg is
Message ? msg.snippet : '$msg';
On 2014/09/03 17:20:41, jakemac wrote:
> I don't think either of these variables are really necessary (same for other
> functions):
> 
> msg = msg is Message ? msg : new Message(
>     MessageId.NOT_SPECIFIED, '$msg');
> _transform.logger.fine(message.snippet, asset: asset, span: span);
> _logs.add(new BuildLogEntry(message, span, LogLevel.FINE.name));

nice, done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/build_logger.dart:89:
JSON.encode(_logs.toJson())));
On 2014/09/03 17:20:41, jakemac wrote:
> I thought JSON.encode will automatically call toJson?

done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/messages/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/messages.dart:34: toString() =>
'${package}_$id';
On 2014/09/03 17:20:41, jakemac wrote:
> imo, a ':' would be better than an '_'

yeah - just trying to use something that doesn't require escaping when we use it
in URLs. I'll experiment with switching to use ':' here and fix it when we
generate the HTML for it.

I'll ping you again when I have that done...

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/messages.dart:63: const Message(this.id,
this.snippet);
On 2014/09/03 17:20:41, jakemac wrote:
> A named constructor or factory that takes no ID and defaults to NOT_SPECIFIED
> might be nice to have, maybe
Message.unknown/Message.notSpecified/Message.simple

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/messages.dart:67: String toString() =>
'${toJson()}';
On 2014/09/03 17:20:41, jakemac wrote:
> I think something like '$id: $snippet' might be better visually than a map.

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/messages.dart:93: /// Short description of
the error message
On 2014/09/03 17:20:41, jakemac wrote:
> How does this differ from snippetTemplate and details?

added details - basically it should be the title we show in generated
documentation, and it can't use placeholders.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:10: const noAbsolutePaths = const
MessageTemplate(
On 2014/09/03 17:20:41, jakemac wrote:
> These should really be all caps names since they are global constants
> (throughout)

:) I think there is not consensus on that part of the style guide, that's why I
didn't use them here (see
https://github.com/dart-lang/www.dartlang.org/issues/831)

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:21: 'Invalid url to reach to another
package: %-url-%. Path '
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> Global change to text:
> url -> URL

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:23: 'way to the %-prefix-% folder.
For example, try changing the url above '
On 2014/09/03 18:56:35, Kathy Walrath wrote:
> is it clear what "above" means?

It seems we didn't need it :). removed

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:23: 'way to the %-prefix-% folder.
For example, try changing the url above '
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> Be consistent about using folder vs. directory. I believe that in the docs we
> usually use "directory".

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:27: To reach an asset that belongs
to another package, we use `package:` URLs in
On 2014/09/03 18:56:35, Kathy Walrath wrote:
> we use -> use
> 
> In general, avoid "we" in messages. It's unclear who "we" is, and usually not
> necessary.

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:28: Dart code, but in any other
language (like HTML or CSS) we use relative URLs.
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> we use -> use

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:30: These are the rules you must
follow to write URLs that refer to files in other
On 2014/09/03 18:56:35, Kathy Walrath wrote:
> -> Follow these rules when writing...
> 
> This is way long, so I'll create a document that contains these rules. (You
can
> ignore the rest of the comments on this message!)

I was debating about this - one idea I was thinking is that we can make such
document the output of compiling this file. Instead of maintaining a separate
place for documentation, each error is documented with the code here.  Note that
the first 3 lines (21-23) is what the user sees in the command-line or pub-serve
as the error message, this part of the message is only visible if they click on
a link to see 'more details'.

It might be easier to chat about this face-to-face, let me know.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:33: * if the file containing the
relative URL is an entrypoint under `web`, use
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> if -> If

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:36: * if the file containing the URL
is under `web`, but on a different directory
On 2014/09/03 18:56:35, Kathy Walrath wrote:
> if -> If
> on -> in

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:40: **Note**: this means that if you
have two entrypoints including this file,
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> this means that if you have two entrypoints including
> ->
> If two entrypoints include
> 
> this file -> the same file that's under `web` (? "this file" didn't seem clear
> enough)

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:44: * if the file containing the URL
lives under `lib`, walk up as many levels as
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> if -> If

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:48: The rules are easier to follow
if you know how the code is layed out for
On 2014/09/03 18:56:35, Kathy Walrath wrote:
> layed -> laid
> 
> (global)

<facepalm> thx!

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:54: '- a1.html
On 2014/09/03 18:56:35, Kathy Walrath wrote:
> Could you use | instead of '? You used | below (for c/web/c1), and it seemed
> clearer to me.

Sure, I was basically using the style that pub uses on the command-line (the
last entry is a bit shorter with ' instead of | )

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:75: If your app is package `c`,
you'll notice that in your file system there is a
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> you'll notice that in your file system there is a packages folder generate in
> the web folder
> ->
> then `pub build` generates a packages folder under the web folder

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:94: `index.html` in Dartium, it will
infer that the package root is under
On 2014/09/03 18:56:35, Kathy Walrath wrote:
> what is "it"? Also, avoid "there is" when possible. Finally, I found myself
> getting a bit confused about whether this was really `pub build` output, since
> no `build` directory was visible. This seemed like it might be just `pub get`
> output.
> 
> Maybe:
> 
> Note that no `lib` directory is under the `packages` directory.
> When Dartium loads `web/index.html`, Dartium infers that the package root
> is under `web/packages/`.
> 
> I'm not sure "infers" is the right word here, but maybe it is. Maybe it's more
> like this:
> 
> ...Dartium loads libraries from package roots directly under `web/packages`.

Good points. Thanks - here is what I ended up with:

Note that no `lib` directory is under the `packages` directory.
When you launch `web/index.html` in Dartium, Dartium loads `package:` imports
from
`web/packages/`.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:154: produce an error message like
this one, instead.
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> This is a lot of doc! How about I put it in a page, so we can have real
figures
> and it's more easily editable. It seems like these messages should give a hint
> and then link to the doc page. 

That's actually how it works, I'm just storing the longer explanation here too. 

The hint is just the second parameter to MessageTemplate. In this case:

  Invalid URL to reach to another package: %-url-%. Path reaching to other
packages must first reach up all the way to the %-prefix-% directory. For
example, try changing the URL to: %-fixedUrl-%.

(where %-name-% are replaced with the tokens from the user's code)

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:161: 'Incomplete import to asset in
another package',
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> to -> of?

rephrased, I should not use the word import here anyways

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:165: requiremnt, but for now we ask
that you use a canonical URL form for it.
On 2014/09/03 18:56:36, Kathy Walrath wrote:
> requiremnt -> requirement

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:168: `packages/b/b.html`, you could
technically just write `../b/b.html`, but we
On 2014/09/03 18:56:35, Kathy Walrath wrote:
> who is "we" really? polymer.dart? The polymer transformer? Maybe it doesn't
> really matter?
> 
> Maybe:
> 
> For example, if `packages/a/a.html` needs to import `packages/b/b.html`,
> you might expect a.html to import `../b/b.html`. Instead, it must import
> `../../packages/b/b.html`.
> See ...

Done. also removed `we` elsewhere...

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
File pkg/observe/lib/src/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:13: 'It should be placed on individual
fields.',
On 2014/09/03 19:36:05, Kathy Walrath wrote:
> It should... fields.
> ->
> Instead, annotate individual fields as @observable.
> [GLOBAL]

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:14: '`@observable` on a library is not
supported.',
On 2014/09/03 19:36:06, Kathy Walrath wrote:
> These messages have a slightly different format from the other messages. To
make
> these consistent, I'll take out the verb and the terminating ".", and (for
> consistency within this group) make the object plural.
> 
> ->
> '`@observable` not supported on libraries',

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:21: '`@observable` on top-level fields is not
supported.',
On 2014/09/03 19:36:05, Kathy Walrath wrote:
> ->
> '`@observable` not supported on top-level fields',

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:28: '`@observable` on classes is not
supported.',
On 2014/09/03 19:36:06, Kathy Walrath wrote:
> ->
> '`@observable` not supported on classes',

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:34: 'Observable fields should be put in
observable objects.',
On 2014/09/03 19:36:05, Kathy Walrath wrote:
> should be put in -> must be in
> [GLOBAL]

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:35: '`@observable` on static fields is not
supported.',
On 2014/09/03 19:36:05, Kathy Walrath wrote:
> ->
> '`@observable` not supported on static fields',

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:40: 'Observable fields should be put in an
observable '
On 2014/09/03 19:36:05, Kathy Walrath wrote:
> an observable -> observable

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:41: 'objects. Please declare that this class
extends from '
On 2014/09/03 19:36:06, Kathy Walrath wrote:
> Please declare -> Change this class to extend, include, or implement
Observable.
> 
> [What does "include Observable" mean?]

include -> mixin

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:44: '`@observable` must be used on a class
that implements `Observable`.',
On 2014/09/03 19:36:06, Kathy Walrath wrote:
> ->
> '`@observable` field not in an `Observable` class',

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/observe/lib/src/mess...
pkg/observe/lib/src/messages.dart:51: must explicitly opt-in on each field you
wish to make observable.
On 2014/09/03 19:36:06, Kathy Walrath wrote:
> I'm not crazy about having the same text for each message. I spent a little
time
> looking at the page to make sure they didn't differ. It might be better to
have
> the common message at the top, directly under "Messages from package observe".
> 
> If we keep the common message:
> 
> Only instance fields on `Observable` classes can be observable,
> and you must explicitly annotate each observable field as `@observable`.
> 
> (That started as just changing "opt-in" to "opt in", but then I realized "opt
> in" might not be clear. It has too many uses of the word "observable", but I
> couldn't think of a clearer way to say it.)

Done. Yeah, I'm not a fan of repeating the message either, but right now I have
no support for package-level docs, just details for each individual message.

Maybe for v2? :)

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:28: * There are two different elements
defined with the same name.
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> There are two -> Two
> defined with -> have [or "are defined that have" or "are defined with"]

went with option 4: `are declared with` :)

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:29: * A single HTML file defining an
element, is imported from two different
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> , is imported from -> has been imported using

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:40: Starting with polymer 0.11.0 we
require that each file that uses the definition
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> 0.11.0 we require that each -> 0.11.0, each

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:41: of polymer-element has to import it
either directly or transitively.
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> has to import -> must import

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:48: 'Please move the import out of this
<polymer-element>.',
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> Please move -> Move

Pretty please? :)

I'm actually curious: since this is an error on the command line, would users
appreciate to hear the magic words?

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:51: HTML imports are expected on the top
of each document, outside of any
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> on the -> at the

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:54: build process will ignore imports
that appear to be in the wrong location.
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> will ignore -> ignores

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:59: 'To run a polymer application, you
need to call "initPolymer". You can '
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> "initPolymer" -> initPolymer()

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:66: We determined that your application
entry point didn't have any Dart script
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> ->
> Your application entrypoint doesn't have...

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:67: tags, and hence, it's missing some
initialization needed for polymer.dart.
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> ->tags, so it's...

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:74: 'No script tags with experimental
bootstrap.',
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> Remove the "."

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:80: 'Single tag per document',
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> Most of these are phrased to describe the problem, not the solution. Maybe:
> 
> Multiple Dart script tags found

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:82: Dartium currently limits to have a
single script tag per document. Any
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> limits to have a single
> ->
> allows only one

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:163: 'Script tags should have either a
URL or an inlined body.',
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> ->
> Script with both src and inline text
> 
> (be sure to remove the ".")

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:165: You have a script tag that includes
both a `src` attribute, and script text
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> attribute,...
> ->
> attribute and inline script text.

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:176: 'Incorrect instantiation: missing
base tag in instantiation.',
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> remove the "."

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:198: This is because browsers first
create the base element, and then upgrade them to
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> them -> it

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:201: doing the upgrade.
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> what is the base type that they need to create
> ->
> which base type it must create

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:212: 'in declaration.',
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> remove the "."

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:225: or that the declaration needs to be
fixed and include the `extends` attribute,
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> or that ->
> Another possibility is that
> 
> and include -> to include

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:293: 'Event handler bindings are method
names, not expressions.',
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> remove the "." and make it not a sentence. Maybe:
> 
> No expressions allowed in event handler bindings

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:303: 'Nested polymer element definitions
are not allowed.',
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> Nested polymer element definitions not allowed
> 
> (removing the verb and the ".")

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:306: `<polymer-element>` defined nested
within a `<polymer-element>`. If you have a
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> defined -> definition?

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:307: definition inside another, please
move the sencond definition out.
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> please move -> move
> sencond -> second

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:309: It's possible that you see this
error if you have an HTML import within a
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> It's possible that you -> You might

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:310: polymer element. You should be able
to also move the import out of the element
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> to also -> to

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:316: 'Missing tag name of the custom
element. Please include an '
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> Please include -> Include

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:318: 'Polymer element definitions must
have a name.',
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> ->
> Polymer element definition without a name

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:329: 'at least one dash and can\'t be
any of the following names: '
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> dash -> dash (-)

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:331: 'Custom element names must have a
dash.',
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> ->
> Custom element name missing a dash

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:333: Custom element names must have a
dash and can\'t be any of the following
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> dash -> dash (`-`)

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:342: 'Failed to inline html import:
%-error-%',
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> html -> HTML

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:343: 'An error occurred while iniling an
import',
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> iniling -> inlining

wow - this typo has been there for months!

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:352: 'An error occurred while iniling a
stylesheet.',
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> ->
> Error while inlining a stylesheet
> 
> (remove the verb and the ".", fix the spelling of inlining)

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:354: An error occurred while iniling a
stylesheet in the polymer build. This is often
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> GLOBAL:
> iniling -> inlining

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:372: 'Not all browsers support bindings
to URL attributes',
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> This message seems more specific than the previous string indicates. They
should
> either both cover URLs or not be so specific to URLs. Maybe:
> 
> 'Attribute missing "_" prefix'

Done

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:374: Not all browsers support bindings
to URL attributes. Some browsers might
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> Delete the first sentence if it just repeats the message. Also, doesn't this
> apply to _style and _value, as well as _src and _href?

Well well well - you just found a bug in the linter :) We were assuming it was
only for URL attributes, not also for value or style. That's why the message was
tailored to URL attributes. We didn't mention URL in the error itself because
the attribute was mentioned already explicitly (%-name-%)

I rephrase it slightly. I kept the first sentence though, because now the title
doesn't include that anymore.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:393: 'The special underscore attribute
is only for bindings.',
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> ->
> Attribute has "_" prefix'

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:410: Sorry, you just run into a bug in
the polymer transformer code. Please file a
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> you just run -> you just ran
> [GLOBAL]

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:411: bug at <http://dartbug.com/new>
including if possible some example code that can
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> including if possible some
> ->
> including, if possible, some
> [GLOBAL]

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:412: help us reproduce the issue so we
can investigate.
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> delete " so we can investigate"

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:432: '`@CustomTag` is only supported on
public classes.',
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> ->
> `@CustomTag` used on a private class

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:435: you need to register a custom
element whose implementation is a private class,
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> Maybe clarify that it's private because its name starts with a _ or its
> library/package names starts with _? Otherwise, Java programmers will think
they
> have to add "public".

added, but I'm not convinced this is the right place to educate about it. Maybe
there is a link about private vs public classes in our docs we can include here?

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:443: '`@initMethod` is only supported on
public functions.',
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> ->
> `@initMethod` used on a private function

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:458: 'Invalid argument in annotation',
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> ->
> Non-constant argument in annotation

Kept as "invalid" because we don't know that the problem was that it was a
non-constant :-/

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:460: We expected a constant argument in
an annotation, but the polymer build was not
On 2014/09/03 21:26:55, Kathy Walrath wrote:
> ->
> The polymer transformer was not able to extract a constant value for the
> annotation argument.
> This can happen...
> 
> (getting rid of "we", plus "build" implies pub build to me, but it could be
pub
> serve, no? but maybe you really didn't mean the transformer?)

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:500: 'Private symbols cannot be used in
event handlers',
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> ->
> Private symbol in event handler

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:502: Currently private members can't be
used on event handlers. So you can't write:
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> on -> as? with? for?

in? :)

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:513: 'Private symbols are not
supported',
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> ->
> Private symbol in a binding expression

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:524: 'A warning was found while parsing
the html document',
On 2014/09/03 21:26:54, Kathy Walrath wrote:
> html -> HTML
> [global within text]

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:526: We use a parser that implements the
HTML5 spec (`html5lib`). We report any
On 2014/09/03 21:26:53, Kathy Walrath wrote:
> We use a
> ->
> The polymer transformer uses a
> 
> We report -> This message reports
> 
> that were detected by our....
> ->
> that the parser detects.

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/tool/create_...
File pkg/polymer/tool/create_message_details_page.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/tool/create_...
pkg/polymer/tool/create_message_details_page.dart:98: font-weight: 700;
On 2014/09/03 17:20:41, jakemac wrote:
> it doesn't look like you are using both of these versions, just font-weight
> normal (not sure which one of these that is actually defaulting to use?). Same
> for roboto.

good question - I copied these from the dartlang site, seems that some elements
like h1 vs p hit one or the other, so we need both.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/tool/create_...
pkg/polymer/tool/create_message_details_page.dart:115: width: 80vw;
On 2014/09/03 17:20:41, jakemac wrote:
> indent

Done.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/tool/create_...
pkg/polymer/tool/create_message_details_page.dart:178: display: inline;
On 2014/09/03 17:20:41, jakemac wrote:
> indent

Done.

[jakemac, 2014-09-04 14:42:37]

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:10: const noAbsolutePaths = const
MessageTemplate(
Weird, but ok :). To me all caps constants is the universal way of defining
constants, so I would personally agree with the style guide (as it stands now)
in this case. I am curious what the argument is against that? The existing bug
doesn't really say why other than it creates a breaking change if you change
something from final to const, which we could leave in an exception for.

On 2014/09/04 02:32:13, Siggi Cherem (dart-lang) wrote:
> On 2014/09/03 17:20:41, jakemac wrote:
> > These should really be all caps names since they are global constants
> > (throughout)
> 
> :) I think there is not consensus on that part of the style guide, that's why
I
> didn't use them here (see
> https://github.com/dart-lang/www.dartlang.org/issues/831)

[Kathy Walrath, 2014-09-04 17:07:28]

lgtm

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/src/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:48: The rules are easier to follow
if you know how the code is layed out for
On 2014/09/04 02:32:14, Siggi Cherem (dart-lang) wrote:
> On 2014/09/03 18:56:35, Kathy Walrath wrote:
> > layed -> laid
> > 
> > (global)
> 
> <facepalm> thx!

I had to look it up to be sure. :)

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/src/messages.dart:154: produce an error message like
this one, instead.
What I meant was I didn't want to bury this doc (about import paths) on this
page. It deserves its own page, which you could link to. (And of course it
shouldn't be in two pages, because then it's likely to get out of sync and
confuse people.) I'll send you a CL today.

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:48: 'Please move the import out of this
<polymer-element>.',
On 2014/09/04 02:32:17, Siggi Cherem (dart-lang) wrote:
> On 2014/09/03 21:26:53, Kathy Walrath wrote:
> > Please move -> Move
> 
> Pretty please? :)
> 
> I'm actually curious: since this is an error on the command line, would users
> appreciate to hear the magic words?

I suppose "Please" isn't bad here. We don't usually use it in docs because it
hides the action. But if people are truly annoyed, "please" can't hurt.

https://codereview.chromium.org/513023002/diff/80001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/messages.dart (right):

https://codereview.chromium.org/513023002/diff/80001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:97: match how they are in Javascript. At
that point the order of your imports with
Javascript -> JavaScript (global)

[Siggi Cherem (dart-lang), 2014-09-04 19:55:12]

Thanks again, ready to submit v0. 

I'll do a follow up CL to support using the generated HTML on the site

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
File pkg/code_transformers/lib/messages/messages.dart (right):

https://codereview.chromium.org/513023002/diff/60001/pkg/code_transformers/li...
pkg/code_transformers/lib/messages/messages.dart:34: toString() =>
'${package}_$id';
On 2014/09/04 02:32:13, Siggi Cherem (dart-lang) wrote:
> On 2014/09/03 17:20:41, jakemac wrote:
> > imo, a ':' would be better than an '_'
> 
> yeah - just trying to use something that doesn't require escaping when we use
it
> in URLs. I'll experiment with switching to use ':' here and fix it when we
> generate the HTML for it.
> 
> I'll ping you again when I have that done...

All set - I went with # here, but do a separate serialization for the HTML tags.

https://codereview.chromium.org/513023002/diff/80001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/messages.dart (right):

https://codereview.chromium.org/513023002/diff/80001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/messages.dart:97: match how they are in Javascript. At
that point the order of your imports with
On 2014/09/04 17:07:28, Kathy Walrath wrote:
> Javascript -> JavaScript (global)

Done.

[Siggi Cherem (dart-lang), 2014-09-04 20:28:34]

Committed patchset #3 (id:100001) manually as 39884 (presubmit successful).