improve import uri completions - fixes #24000

improve import uri completions - fixes #24000

R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/773c3cf9d107198cf8f45cb8753237cc63e3b322

[danrubel, 2015-08-30 02:41:49]

danrubel@google.com changed reviewers:
+ brianwilkerson@google.com

[Brian Wilkerson, 2015-08-30 16:26:59]

LGTM

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/completion/import_uri_contributor.dart
(right):

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/import_uri_contributor.dart:25:
class ImportUriContributor extends DartCompletionContributor {
Given that this is (according to the comment) used for both import and part
directives (but not export? perhaps the comment needs to be updated) consider
renaming it to "UriContributor".

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/test/se...
File
pkg/analysis_server/test/services/completion/import_uri_contributor_test.dart
(right):

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/import_uri_contributor_test.dart:287:
class ImportUriContributorWindowsTest extends AbstractCompletionTest {
Suggestion: Move all of the file-based tests into a separate test class
(FileUriContributorTest?), then have one subclass
(WindowsFileUriContributorTest) that creates a Windows-based resource provider
but doesn't implement any additional tests. All of the tests from the superclass
will be run as if they were also defined for the subclass. That way (a) we avoid
duplication and (b) we avoid forgetting to copy tests later.

You might need to override addSource in the subclass to convert a posix file
path to a Windows file path, but the code on line 436 implies that that won't be
necessary.

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/import_uri_contributor_test.dart:436:
addSource('/proj/other.dart', 'library other;');
If we can't have the single abstract class suggested above, then I think these
separators need to be fixed.

[danrubel, 2015-08-31 19:21:41]

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

[danrubel, 2015-08-31 19:52:03]

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/services/completion/import_uri_contributor.dart
(right):

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/import_uri_contributor.dart:25:
class ImportUriContributor extends DartCompletionContributor {
On 2015/08/30 16:26:59, Brian Wilkerson wrote:
> Given that this is (according to the comment) used for both import and part
> directives (but not export? perhaps the comment needs to be updated) consider
> renaming it to "UriContributor".

Good point. https://codereview.chromium.org/1329463002/

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/test/se...
File
pkg/analysis_server/test/services/completion/import_uri_contributor_test.dart
(right):

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/import_uri_contributor_test.dart:287:
class ImportUriContributorWindowsTest extends AbstractCompletionTest {
On 2015/08/30 16:26:59, Brian Wilkerson wrote:
> Suggestion: Move all of the file-based tests into a separate test class
> (FileUriContributorTest?), then have one subclass
> (WindowsFileUriContributorTest) that creates a Windows-based resource provider
> but doesn't implement any additional tests. All of the tests from the
superclass
> will be run as if they were also defined for the subclass. That way (a) we
avoid
> duplication and (b) we avoid forgetting to copy tests later.
> 
> You might need to override addSource in the subclass to convert a posix file
> path to a Windows file path, but the code on line 436 implies that that won't
be
> necessary.

Per discussion, we'll slowly move to a single test suite that uses the
platform's path separator. I'll keep this as two separate classes for this CL,
but merge in a subsequent CL as I work out the details.

https://codereview.chromium.org/1325583002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/import_uri_contributor_test.dart:436:
addSource('/proj/other.dart', 'library other;');
On 2015/08/30 16:26:59, Brian Wilkerson wrote:
> If we can't have the single abstract class suggested above, then I think these
> separators need to be fixed.

Good point. Fixed.