Support optional package name arguments to --show-package-warnings

Support optional package name arguments to --show-package-warnings

In response to dartbug.com/18661 and dartbug.com/21651.

Dart2js doesn't know pubspec.yaml or the directory structure but pub does. Next step is to pass arguments from pub.

BUG=
R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/212ba4afbec225417fa36ceee741bc9f9cb9b616

[Johnni Winther, 2015-12-10 14:59:03]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Johnni Winther, 2015-12-10 14:59:04]

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/partial_elements.dart (right):

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/partial_elements.dart:301: return
resolution.resolveTypeAnnotation(element, node.type);
Bug found during implementation. We could point to the wrong file when reporting
an error from resolving the type annotation.

https://codereview.chromium.org/1513263002/diff/1/tests/compiler/dart2js/show...
File tests/compiler/dart2js/show_package_warnings_test.dart (left):

https://codereview.chromium.org/1513263002/diff/1/tests/compiler/dart2js/show...
tests/compiler/dart2js/show_package_warnings_test.dart:51: var options =
[Flags.analyzeOnly, Flags.analyzeAll];
Removed analyzeAll and insert calls of error code to make the test faster.

[Siggi Cherem (dart-lang), 2015-12-10 16:21:43]

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/diagno...
File pkg/compiler/lib/src/diagnostics/diagnostic_listener.dart (right):

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/diagno...
pkg/compiler/lib/src/diagnostics/diagnostic_listener.dart:57:
_shownPackageWarnings.contains(uri.path);
should we check for the first segment or a prefix rather than the full Uri path?
For example,

  _shownPackageWarnings.any(uri.path.startsWith)

or 
  _shownPackageWarnings.any(uri.pathSegments[0])

?

I'm thinking that people will commonly say `--show-package-warnings=unittest` to
show all warnings from the unittest package, and here we are checking for
something like `unittest/unittest.dart`. So the full path won't be in the list. 

(I may be missing something, because your test seem to match my expectations)

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/partial_elements.dart (right):

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/partial_elements.dart:301: return
resolution.resolveTypeAnnotation(element, node.type);
On 2015/12/10 14:59:04, Johnni Winther wrote:
> Bug found during implementation. We could point to the wrong file when
reporting
> an error from resolving the type annotation.

Do you think it is worth looking into
https://github.com/dart-lang/sdk/issues/3675 to prevent these?

[Johnni Winther, 2015-12-11 10:33:28]

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/diagno...
File pkg/compiler/lib/src/diagnostics/diagnostic_listener.dart (right):

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/diagno...
pkg/compiler/lib/src/diagnostics/diagnostic_listener.dart:57:
_shownPackageWarnings.contains(uri.path);
On 2015/12/10 16:21:42, Siggi Cherem (dart-lang) wrote:
> should we check for the first segment or a prefix rather than the full Uri
path?
> For example,
> 
>   _shownPackageWarnings.any(uri.path.startsWith)
> 
> or 
>   _shownPackageWarnings.any(uri.pathSegments[0])
> 
> ?
> 
> I'm thinking that people will commonly say `--show-package-warnings=unittest`
to
> show all warnings from the unittest package, and here we are checking for
> something like `unittest/unittest.dart`. So the full path won't be in the
list. 
> 
> (I may be missing something, because your test seem to match my expectations)

Done; I was going to put `_shownPackageWarnings.any(uri.pathSegments.first)` but
got distracted while waiting for code completion :(

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/partial_elements.dart (right):

https://codereview.chromium.org/1513263002/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/partial_elements.dart:301: return
resolution.resolveTypeAnnotation(element, node.type);
On 2015/12/10 16:21:42, Siggi Cherem (dart-lang) wrote:
> On 2015/12/10 14:59:04, Johnni Winther wrote:
> > Bug found during implementation. We could point to the wrong file when
> reporting
> > an error from resolving the type annotation.
> 
> Do you think it is worth looking into
> https://github.com/dart-lang/sdk/issues/3675 to prevent these?

I'll take a (new) look at 3675.

[Johnni Winther, 2015-12-14 10:37:14]

ping

[Siggi Cherem (dart-lang), 2015-12-14 22:32:02]

lgtm

[Johnni Winther, 2015-12-15 09:52:25]

Description was changed from

==========
Support optional package name arguments to --show-package-warnings

In response to dartbug.com/18661 and dartbug.com/21651.

Dart2js doesn't know pubspec.yaml or the directory structure but pub does. Next
step is to pass arguments from pub.

BUG=
==========

to

==========
Support optional package name arguments to --show-package-warnings

In response to dartbug.com/18661 and dartbug.com/21651.

Dart2js doesn't know pubspec.yaml or the directory structure but pub does. Next
step is to pass arguments from pub.

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/212ba4afbec225417fa36ceee741bc9f9cb9b616
==========

[Johnni Winther, 2015-12-15 09:52:27]

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