Checks for entry point existence and supports warning suppression.

Checks for entry point existence and supports warning suppression.

We've had issue #17 forever, because we have no way to check whether a certain entry point actually exists (we do not know the root directory of a package when it is transformed).

This CL checks whether a specified entry point file exists, which eliminates all the warnings previously given for entry points in the local package (the one where `pub build..` or `pub serve..` is executed) because of the arguments given to pub (we would warn about missing 'test/*.dart' when running `pub build web`, because pub does not deliver 'test/*.dart' to the transformer in that case).

However, this approach used to fail for transformation of other packages (the ones that the local package depends on), because the root directory of such a package is unknown (and it is not the current working directory of the process that runs the transformer). However, I have made many attempts to reproduce this problem, and it never occurred. It is possible that `pub` has been modified.

On top of that, this CL provides support for suppression of warnings, using a setting in 'pubspec.yaml'.

Those two elements together should make it possible for users of this package to avoid the Info message about 'Missing entry point', and we may then close the issue.

R=sigurdm@google.com

Committed: https://github.com/dart-lang/reflectable/commit/2dc5e46ef6aa17daab9240a73f79fd7364c76193

[eernst, 2015-11-17 13:48:56]

Description was changed from

==========
Checks for entry point existence and supports warning suppression.
==========

to

==========
Checks for entry point existence and supports warning suppression.

We've had issue #17 forever, because we have no way to check whether a certain
entry point actually exists (we do not know the root directory of a package when
it is transformed).

This CL checks whether a specified entry point file exists, which eliminates all
the warnings previously given for entry points in the local package (the one
where `pub build..` or `pub serve..` is executed) because of the arguments given
to pub (we would warn about missing 'test/*.dart' when running `pub build web`,
because pub does not deliver 'test/*.dart' to the transformer in that case).

However, this approach used to fail for transformation of other packages (the
ones that the local package depends on), because the root directory of such a
package is unknown (and it is not the current working directory of the process
that runs the transformer). However, I have made many attempts to reproduce this
problem, and it never occurred. It is possible that `pub` has been modified.

On top of that, this CL provides support for suppression of warnings, using a
setting in 'pubspec.yaml'.

Those two elements together should make it possible for users of this package to
avoid the Info message about 'Missing entry point', and we may then close the
issue.
==========

[eernst, 2015-11-17 13:49:37]

eernst@google.com changed reviewers:
+ sigurdm@google.com

[eernst, 2015-11-17 13:49:37]

Dealing with #17 (unwanted warnings about 'Missing entry point').

[sigurdm, 2015-11-17 14:05:21]

LGTM

https://codereview.chromium.org/1448333004/diff/1/reflectable/lib/src/transfo...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1448333004/diff/1/reflectable/lib/src/transfo...
reflectable/lib/src/transformer_implementation.dart:30: allWarnings,
allWarnings is not really a kind of warning.

Why not translate allWarnings to a list with all the kinds in
findSuppressWarnings?

[eernst, 2015-11-17 14:38:13]

Review response.

https://codereview.chromium.org/1448333004/diff/1/reflectable/lib/src/transfo...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1448333004/diff/1/reflectable/lib/src/transfo...
reflectable/lib/src/transformer_implementation.dart:30: allWarnings,
On 2015/11/17 14:05:21, sigurdm wrote:
> allWarnings is not really a kind of warning.
> 
> Why not translate allWarnings to a list with all the kinds in
> findSuppressWarnings?

Did think about that, and it is indeed a more regular approach in this case
where the full set can easily be represented. Done.

[eernst, 2015-11-17 14:48:04]

Committed patchset #2 (id:20001) manually as
2dc5e46ef6aa17daab9240a73f79fd7364c76193 (presubmit successful).

[jakemac, 2015-11-17 14:58:06]

jakemac@google.com changed reviewers:
+ jakemac@google.com

[jakemac, 2015-11-17 14:58:07]

https://codereview.chromium.org/1448333004/diff/20001/reflectable/lib/src/tra...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1448333004/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:3026: bool
entryPointFileExists = await new File(entryPointPath).exists();
This is definitely an anti-pattern for transformers in general... I guess it
doesn't hurt though as long as you are checking for the entry point in the
provided assets first (it could be generated by a previous phase, and would not
live on disk).

[eernst, 2015-11-17 15:22:40]

On 2015/11/17 14:58:07, jakemac wrote:
>
https://codereview.chromium.org/1448333004/diff/20001/reflectable/lib/src/tra...
> File reflectable/lib/src/transformer_implementation.dart (right):
> 
>
https://codereview.chromium.org/1448333004/diff/20001/reflectable/lib/src/tra...
> reflectable/lib/src/transformer_implementation.dart:3026: bool
> entryPointFileExists = await new File(entryPointPath).exists();
> This is definitely an anti-pattern for transformers in general... I guess it
> doesn't hurt though as long as you are checking for the entry point in the
> provided assets first (it could be generated by a previous phase, and would
not
> live on disk).

You're right that transformers shouldn't bypass an asset and read from disk.
However, in this particular situation each of the 'entry_points:' is written
manually by a programmer, and the package documentation describes the meaning of
such an entry as 'The path to your main file', and there is no corresponding
asset (we only reach this point because the asset is missing). So the entry
point really does refer to the file. On top of that we will not do anything with
the file, we just detect that it exists in order to suppress a message, and then
never touch it again. So we won't incorrectly bypass an asset here.

[jakemac, 2015-11-17 15:26:58]

On 2015/11/17 15:22:40, eernst wrote:
> On 2015/11/17 14:58:07, jakemac wrote:
> >
>
https://codereview.chromium.org/1448333004/diff/20001/reflectable/lib/src/tra...
> > File reflectable/lib/src/transformer_implementation.dart (right):
> > 
> >
>
https://codereview.chromium.org/1448333004/diff/20001/reflectable/lib/src/tra...
> > reflectable/lib/src/transformer_implementation.dart:3026: bool
> > entryPointFileExists = await new File(entryPointPath).exists();
> > This is definitely an anti-pattern for transformers in general... I guess it
> > doesn't hurt though as long as you are checking for the entry point in the
> > provided assets first (it could be generated by a previous phase, and would
> not
> > live on disk).
> 
> You're right that transformers shouldn't bypass an asset and read from disk.
> However, in this particular situation each of the 'entry_points:' is written
> manually by a programmer, and the package documentation describes the meaning
of
> such an entry as 'The path to your main file', and there is no corresponding
> asset (we only reach this point because the asset is missing). So the entry
> point really does refer to the file. On top of that we will not do anything
with
> the file, we just detect that it exists in order to suppress a message, and
then
> never touch it again. So we won't incorrectly bypass an asset here.

Ya, I think I agree that this is an exception to the general rule, only because
you aren't actually using this for anything other than as an attempt to not emit
false warnings. I am sure there are some situations in which it won't work, but
it will prevent the warning in the general case and won't make those cases any
worse than they are already.