Implemented analysis.setPrioritySource

Implemented analysis.setPrioritySource

R=paulberry@google.com

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

[Paul Berry, 2014-06-16 23:43:16]

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

https://codereview.chromium.org/332993006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:177: void
setPriorityFiles(Request request, List<String> files) {
I don't think this algorithm will do the right thing when the set of priority
sources for a given context drops to zero.  For example, consider this sequence
of commands:

- setPriorityFiles(['ctx1/foo.dart', 'ctx2/bar.dart'])
  - causes ctx1's priority order to be set to ['ctx1/foo.dart'] and ctx2's
priority order to be set to ['ctx2/bar.dart']
- setPriorityFiles(['ctx2/bar.dart', 'ctx2/baz.dart'])
  - causes ctx2's priority order to be set to ['ctx2/bar.dart', 'ctx2/baz.dart']
  - fails to clear ctx1's priority order
- setPriorityFiles([])
  - fails to clear ctx2's priority order

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

https://codereview.chromium.org/332993006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_edit.dart:175: }
RequestDatum was intended to make this sort of error handling logic unnecessary
(so that we don't accidentally leave out necessary error checks).  You should be
able to replace everything from datum.forEachMap() through here with this
instead:

  file = datum[FILE].asString();
  errorCodeName = datum[ERROR_CODE].asString();
  offset = datum[OFFSET].asInt();
  length = datum[LENGTH].asInt();
  message = datum[MESSAGE].asString();
  if (datum.hasKey(CORRECTION)) {
    correction = datum[CORRECTION].asString();
  }

Datum's operator[], asString(), and asInt() methods will do all of the necessary
error checking, and throw exceptions if the data wasn't present or had the wrong
type.  If no exception is thrown, you are guaranteed that every call to
asString() returned a string, and every call to asInt() returned an int.

https://codereview.chromium.org/332993006/diff/1/pkg/analysis_server/test/dom...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/332993006/diff/1/pkg/analysis_server/test/dom...
pkg/analysis_server/test/domain_analysis_test.dart:106: });
At a minimum, we should have a test to verify that setPriorityFiles([]) clears
the priority lists for all contexts that previously had non-empty priority
lists.

Depending on the implementation you choose, it might also be good to verify that
setPriorityFiles(list of files from context A) properly clears the priority list
for context B.

[Brian Wilkerson, 2014-06-17 17:46:16]

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

https://codereview.chromium.org/332993006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:177: void
setPriorityFiles(Request request, List<String> files) {
> I don't think this algorithm will do the right thing when the set of priority
> sources for a given context drops to zero.

Good catch! Thanks!

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

https://codereview.chromium.org/332993006/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_edit.dart:175: }
Thanks! That's much cleaner.

https://codereview.chromium.org/332993006/diff/1/pkg/analysis_server/test/dom...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/332993006/diff/1/pkg/analysis_server/test/dom...
pkg/analysis_server/test/domain_analysis_test.dart:106: });
> At a minimum, we should have a test to verify that setPriorityFiles([]) clears
> the priority lists for all contexts that previously had non-empty priority
> lists.

Agreed.

[Brian Wilkerson, 2014-06-17 17:46:27]

PTAL

[Paul Berry, 2014-06-17 18:04:48]

lgtm

https://codereview.chromium.org/332993006/diff/20001/pkg/analysis_server/test...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/332993006/diff/20001/pkg/analysis_server/test...
pkg/analysis_server/test/domain_analysis_test.dart:107: //
expect(server.getPriorityFiles(), equals(fileList));
I think this needs to be

  expect(server.getPriorityFiles(), unorderedEquals(fileList));

since there's no guarantee of the order of the list returned by
server.getPriorityFiles().

[Brian Wilkerson, 2014-06-17 18:43:56]

https://codereview.chromium.org/332993006/diff/20001/pkg/analysis_server/test...
File pkg/analysis_server/test/domain_analysis_test.dart (right):

https://codereview.chromium.org/332993006/diff/20001/pkg/analysis_server/test...
pkg/analysis_server/test/domain_analysis_test.dart:107: //
expect(server.getPriorityFiles(), equals(fileList));
Done

[Brian Wilkerson, 2014-06-17 18:44:34]

Committed patchset #2 manually as r37412 (presubmit successful).