A local file-based Index implementation.

A local file-based Index implementation.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Brian Wilkerson, 2014-06-24 13:53:41]

LGTM

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/index/index.dart (right):

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/index.dart:51: Future<List<Location>>
getRelationshipsAsync(Element element,
You should double check, but I believe that the convention is for asynchronous
methods to have the simple name and for synchronous versions to have a suffix of
"Sync".

Also, this method needs a comment.

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/res...
File pkg/analysis_server/test/resource_test.dart (right):

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/res...
pkg/analysis_server/test/resource_test.dart:435: void
_testResourceResourceUriResolver() {
I don't understand the value of private names in test code.

[scheglov, 2014-06-24 15:21:23]

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/index/index.dart (right):

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/index.dart:51: Future<List<Location>>
getRelationshipsAsync(Element element,
On 2014/06/24 13:53:41, Brian Wilkerson wrote:
> You should double check, but I believe that the convention is for asynchronous
> methods to have the simple name and for synchronous versions to have a suffix
of
> "Sync".

  I have to implementIndex interface as is for now.
  But yes, maybe it worth to make changes to it sooner than after complete
migration from Java.


> 
> Also, this method needs a comment.

Done.

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/res...
File pkg/analysis_server/test/resource_test.dart (right):

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/res...
pkg/analysis_server/test/resource_test.dart:435: void
_testResourceResourceUriResolver() {
On 2014/06/24 13:53:41, Brian Wilkerson wrote:
> I don't understand the value of private names in test code.

Done.

[Paul Berry, 2014-06-24 15:49:59]

lgtm

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/index/index.dart (right):

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/index.dart:51: Future<List<Location>>
getRelationshipsAsync(Element element,
On 2014/06/24 13:53:41, Brian Wilkerson wrote:
> You should double check, but I believe that the convention is for asynchronous
> methods to have the simple name and for synchronous versions to have a suffix
of
> "Sync".

I agree in principle, however since the synchronous getRelationships() is
inherited from java2dart translated code, I would personally be ok with leaving
this as is until after we stop translating.

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/resource.dart (right):

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:473: final MemoryResourceProvider
_provider;
It doesn't look like we're making use of any functionality that's specific to
MemoryResourceProvider.  Let's change this to just:

  final ResourceProvider _provider;

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
File pkg/analysis_server/test/index/index_test.dart (right):

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
pkg/analysis_server/test/index/index_test.dart:51: MemoryResourceProvider
provider = new MemoryResourceProvider();
This should be initialized in setUp().

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
pkg/analysis_server/test/index/index_test.dart:71: test_clear() {
Since it is a common mistake to forget to return newly created futures
(especially in tests), I think it would be really helpful to annotate the return
types of all our tests (i.e. make this "Future test_clear() {")

A similar comment applies to test_indexUnit(), test_removeContext(),
test_removeSource(), and test_removeSources().

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
pkg/analysis_server/test/index/index_test.dart:88: }
We should also test that getRelationships() passes the correct set of
relationships to the callback when the set of relationships is non-empty.

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
pkg/analysis_server/test/index/index_test.dart:90: test_indexHtmlUnit_nullUnit()
{
Similarly, I'd prefer to see this function's return type annotated as void. 
Same goes for test_indexHtmlUnit_nullUnitElement(), test_indexUnit_nullUnit(),
and test_indexUnit_nullUnitElement().

[scheglov, 2014-06-24 18:07:46]

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/resource.dart (right):

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/resource.dart:473: final MemoryResourceProvider
_provider;
On 2014/06/24 15:49:59, Paul Berry wrote:
> It doesn't look like we're making use of any functionality that's specific to
> MemoryResourceProvider.  Let's change this to just:
> 
>   final ResourceProvider _provider;

Done.

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
File pkg/analysis_server/test/index/index_test.dart (right):

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
pkg/analysis_server/test/index/index_test.dart:51: MemoryResourceProvider
provider = new MemoryResourceProvider();
On 2014/06/24 15:49:59, Paul Berry wrote:
> This should be initialized in setUp().

Why?
It does not reference any other field, so can be initialized in declaration.

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
pkg/analysis_server/test/index/index_test.dart:71: test_clear() {
On 2014/06/24 15:49:59, Paul Berry wrote:
> Since it is a common mistake to forget to return newly created futures
> (especially in tests), I think it would be really helpful to annotate the
return
> types of all our tests (i.e. make this "Future test_clear() {")
> 
> A similar comment applies to test_indexUnit(), test_removeContext(),
> test_removeSource(), and test_removeSources().

I'm not 100% sure that declaring Future explicitly will help.
If we forget to return the created Future, we will forget to declare that
methods returns it too :-)

But I think it is reasonable to have an explicit return type instead of dynamic,
so done.

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
pkg/analysis_server/test/index/index_test.dart:88: }
On 2014/06/24 15:49:59, Paul Berry wrote:
> We should also test that getRelationships() passes the correct set of
> relationships to the callback when the set of relationships is non-empty.

Actually we cannot.
getRelationships() implements Index API, but does nothing.
We cannot (or at least I don't know how) to convert async SplitIndexStore API we
use into synchronous one.

https://codereview.chromium.org/351813002/diff/1/pkg/analysis_server/test/ind...
pkg/analysis_server/test/index/index_test.dart:90: test_indexHtmlUnit_nullUnit()
{
On 2014/06/24 15:49:59, Paul Berry wrote:
> Similarly, I'd prefer to see this function's return type annotated as void. 
> Same goes for test_indexHtmlUnit_nullUnitElement(), test_indexUnit_nullUnit(),
> and test_indexUnit_nullUnitElement().

Done, thank you for catching this.

[scheglov, 2014-06-24 18:08:08]

Committed patchset #3 manually as r37664 (presubmit successful).