Add toMap / toMapOf / toList

Add toMap / toMapOf / toList

Rename 'inputOf' to 'of'.

R=brianwilkerson@google.com
BUG=

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

[Brian Wilkerson, 2015-03-24 15:07:11]

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:188: MapTaskInput _lutMapInput(Source
libSource, ResultDescriptor result) {
I'd really prefer a name that didn't include an abbreviation ("lut") I'll never
be able to remember. Given that it's only used in one place, I'd recommend
inlining it.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:452: lut(Source unit) => new
LibraryUnitTarget(libSource, unit);
I don't understand the point of defining a local method that's only used in one
place (especially when the name of the function doesn't add semantic content).

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:1544: lut(Source unit) => new
LibraryUnitTarget(libSource, unit);
Even if it's useful to abbreviate the code here (which isn't clear to me), we
need a meaningful name (which would probably negate the value of defining the
local function). Brevity doesn't always improve readability.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/i...
File pkg/analyzer/lib/src/task/inputs.dart (right):

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/i...
pkg/analyzer/lib/src/task/inputs.dart:37: TaskInput<Map<E, Object>>
toMap(GenerateTaskInputs mapper) {
The overridden method has a parameter type of MapTaskInput. I think this needs
to be changed.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
File pkg/analyzer/lib/task/model.dart (right):

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:1: // Copyright (c) 2015, the Dart project
authors.  Please see the AUTHORS file
This needs to be sorted.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:34: typedef TaskInput<E> MapTaskInput<B, E>(B
object);
The name makes it sound like this function is mapping a TaskInput to something
else, but it isn't. Given the comments below, perhaps the best path is some
incredibly generic name, like UnaryFunction (or getting rid of it entirely).

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:282: TaskInputBuilder<List<E>> createBuilder();
We shouldn't need to override this method, but if we do we should mark it as
@override and remove the comment.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:286: * this input into some other objects.
Perhaps:

Return a task input that can be used to compute a map whose keys are the
elements of this input and whose values are the result of passing the
corresponding key to the [mapper] function.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:288: TaskInput<Map<E, Object>>
toMap(MapTaskInput mapper);
Given that MapTaskInput returns a TaskInput, shouldn't this be "TaskInput<Map<E,
TaskInput>>"?

But the bigger question is, why restrict clients to building a map whose values
are task inputs? Why not allow a Map<AnalysisTarget, SourceKind>, for example?
Perhaps MapTaskInput should be defined as

typedef R MapTaskInput<P, R>(P parameter);

and someday this method could be

TaskInput<Map<E, R>> toMap<R>(MapTaskInput<E, R> f);

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:292: * of this input.
Perhaps:

Return a task input that can be used to compute a list whose elements are the
result of passing the elements of this input to the [mapper] function.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:294: TaskInput<List> toList(MapTaskInput
mapper);
Return ListTaskInput, so that they can be chained?

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:298: * this input into [valueResult]s.
Perhaps:

Return a task input that can be used to compute a map whose keys are the
elements of this input and whose values are the [valueResult]'s associated with
those elements.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:301: }
Seems like a corresponding method would be good:

ListTaskInput/*<E>*/ toListOf/*<E>*/(ResultDescriptor/*<E>*/ result);

[scheglov, 2015-03-24 17:16:39]

PTAL

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/d...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:188: MapTaskInput _lutMapInput(Source
libSource, ResultDescriptor result) {
On 2015/03/24 15:07:10, Brian Wilkerson wrote:
> I'd really prefer a name that didn't include an abbreviation ("lut") I'll
never
> be able to remember. Given that it's only used in one place, I'd recommend
> inlining it.

Done.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:452: lut(Source unit) => new
LibraryUnitTarget(libSource, unit);
On 2015/03/24 15:07:10, Brian Wilkerson wrote:
> I don't understand the point of defining a local method that's only used in
one
> place (especially when the name of the function doesn't add semantic content).

Done.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/d...
pkg/analyzer/lib/src/task/dart.dart:1544: lut(Source unit) => new
LibraryUnitTarget(libSource, unit);
On 2015/03/24 15:07:10, Brian Wilkerson wrote:
> Even if it's useful to abbreviate the code here (which isn't clear to me), we
> need a meaningful name (which would probably negate the value of defining the
> local function). Brevity doesn't always improve readability.

Given that this local function is declared 3 lines above its usage, semantically
highlighted and named uniformly everywhere - in the source and docs, I'd expect
it to be pretty clear.

Fixed for now.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/i...
File pkg/analyzer/lib/src/task/inputs.dart (right):

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/src/task/i...
pkg/analyzer/lib/src/task/inputs.dart:37: TaskInput<Map<E, Object>>
toMap(GenerateTaskInputs mapper) {
On 2015/03/24 15:07:10, Brian Wilkerson wrote:
> The overridden method has a parameter type of MapTaskInput. I think this needs
> to be changed.

Done.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
File pkg/analyzer/lib/task/model.dart (right):

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:1: // Copyright (c) 2015, the Dart project
authors.  Please see the AUTHORS file
On 2015/03/24 15:07:11, Brian Wilkerson wrote:
> This needs to be sorted.

Done.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:34: typedef TaskInput<E> MapTaskInput<B, E>(B
object);
On 2015/03/24 15:07:11, Brian Wilkerson wrote:
> The name makes it sound like this function is mapping a TaskInput to something
> else, but it isn't. Given the comments below, perhaps the best path is some
> incredibly generic name, like UnaryFunction (or getting rid of it entirely).

MapToTaskInput ?
ToTaskInput ?

UnaryFunction for now.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:282: TaskInputBuilder<List<E>> createBuilder();
On 2015/03/24 15:07:10, Brian Wilkerson wrote:
> We shouldn't need to override this method, but if we do we should mark it as
> @override and remove the comment.

Done.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:286: * this input into some other objects.
On 2015/03/24 15:07:11, Brian Wilkerson wrote:
> Perhaps:
> 
> Return a task input that can be used to compute a map whose keys are the
> elements of this input and whose values are the result of passing the
> corresponding key to the [mapper] function.

Done.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:288: TaskInput<Map<E, Object>>
toMap(MapTaskInput mapper);
On 2015/03/24 15:07:11, Brian Wilkerson wrote:
> Given that MapTaskInput returns a TaskInput, shouldn't this be
"TaskInput<Map<E,
> TaskInput>>"?

  Not quite.
  It's final result is Map<E, V>, where V is result of TaskInput<V>, which is
created be the mapper.

> 
> But the bigger question is, why restrict clients to building a map whose
values
> are task inputs? Why not allow a Map<AnalysisTarget, SourceKind>, for example?
> Perhaps MapTaskInput should be defined as
> 
> typedef R MapTaskInput<P, R>(P parameter);
> 
> and someday this method could be
> 
> TaskInput<Map<E, R>> toMap<R>(MapTaskInput<E, R> f);

  Well, toMap() and toMapOf() are used to create specifically
ListToMapTaskInput, which expects "TaskInput<E> GenerateTaskInputs<B, E>(B
object)", so we need the mapper to return TaskInput.

  While it would be nice to have a general case mapping, it is not strongly
required be implementation.
  We can do other kinds of mappings in a task itself.
  But when we want values of a TaskInputs, this requiring looking into a
CacheEntry, so it should be done externally, and cannot be done inside of a
task.

  There is also no a good way (without using mirrors) to distinguish between
mappers returning TaskInput(s) or final values.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:292: * of this input.
On 2015/03/24 15:07:11, Brian Wilkerson wrote:
> Perhaps:
> 
> Return a task input that can be used to compute a list whose elements are the
> result of passing the elements of this input to the [mapper] function.

Done.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:294: TaskInput<List> toList(MapTaskInput
mapper);
On 2015/03/24 15:07:11, Brian Wilkerson wrote:
> Return ListTaskInput, so that they can be chained?

Later.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:298: * this input into [valueResult]s.
On 2015/03/24 15:07:11, Brian Wilkerson wrote:
> Perhaps:
> 
> Return a task input that can be used to compute a map whose keys are the
> elements of this input and whose values are the [valueResult]'s associated
with
> those elements.

Done.

https://codereview.chromium.org/1005693007/diff/1/pkg/analyzer/lib/task/model...
pkg/analyzer/lib/task/model.dart:301: }
On 2015/03/24 15:07:11, Brian Wilkerson wrote:
> Seems like a corresponding method would be good:
> 
> ListTaskInput/*<E>*/ toListOf/*<E>*/(ResultDescriptor/*<E>*/ result);

Done.

[Brian Wilkerson, 2015-03-25 15:55:21]

LGTM

[scheglov, 2015-03-25 17:54:30]

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