Issue 118783003: Command class hieracy refactoring, environmentOverride extraction

Issue 118783003: Command class hieracy refactoring, environmentOverride extraction (Closed)

Created:
7 years ago by kustermann

Modified:
7 years ago

Reviewers:
ricow1, Bill Hesse

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Command class hieracy refactoring, environmentOverride extraction R=ricow@google.com, whesse@google.com Committed: https://code.google.com/p/dart/source/detail?r=31269

Patch Set 3 : #

Created: 7 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+178 lines, -152 lines)			Patch
M	dart/tests/standalone/io/skipping_dart2js_compilations_test.dart	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	dart/tests/standalone/io/test_runner_test.dart	View		1 chunk	+4 lines, -4 lines	0 comments	Download
M	dart/tools/testing/dart/co19_test.dart	View		1 chunk	+0 lines, -4 lines	0 comments	Download
M	dart/tools/testing/dart/record_and_replay.dart	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	dart/tools/testing/dart/test_runner.dart	View	1 2	16 chunks	+119 lines, -116 lines	0 comments	Download
M	dart/tools/testing/dart/test_suite.dart	View		13 chunks	+22 lines, -24 lines	0 comments	Download
M	dart/tools/testing/dart/utils.dart	View	1 2	1 chunk	+30 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

ricow1

LGTM https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/test_runner.dart File dart/tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/test_runner.dart#newcode118 dart/tools/testing/dart/test_runner.dart:118: // TODO(efortuna): Remove this when fixed (Issue 1306). ...

7 years ago (2013-12-19 10:58:26 UTC) #2

Bill Hesse

lgtm https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/test_runner.dart File dart/tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/test_runner.dart#newcode87 dart/tools/testing/dart/test_runner.dart:87: bool _equal(Command other) { Why not return hashCode ...

7 years ago (2013-12-19 11:00:00 UTC) #3

kustermann

https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/test_runner.dart File dart/tools/testing/dart/test_runner.dart (right): https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/test_runner.dart#newcode87 dart/tools/testing/dart/test_runner.dart:87: bool _equal(Command other) { On 2013/12/19 11:00:00, Bill Hesse ...

7 years ago (2013-12-19 12:38:00 UTC) #4

https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/...
File dart/tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_runner.dart:87: bool _equal(Command other) {
On 2013/12/19 11:00:00, Bill Hesse wrote:
> Why not 
> return hashCode == other.hashCode && commandLine == ... ;

Done.

https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_runner.dart:116: // Windows can't handle the first
command if it is a .bat file or the like
On 2013/12/19 11:00:00, Bill Hesse wrote:
> I'm sure this is fixed.  The issue is closed, and we are giving actual
> filesystem path strings.

I won't change this in this CL.

https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_runner.dart:118: // TODO(efortuna): Remove this
when fixed (Issue 1306).
On 2013/12/19 10:58:26, ricow1 wrote:
> do we need this, 1306 is marked as fixed

It's good to have an issue number to see why it was introduced in the first
place.

https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_runner.dart:124: builder.add(executable);
On 2013/12/19 10:58:26, ricow1 wrote:
> shouldn't you call super._buildHashCode here?

Good catch :)

But it doesn't matter much, since the ProcessCommand class holds all of the
information that is unique to this command.
The only exception is "displayName" -- which we actually shouldn't take into
account, but that's another story.

https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_runner.dart:151: environmentOverrides.length !=
other.environmentOverrides.length) {
On 2013/12/19 11:00:00, Bill Hesse wrote:
> Extract map comparison and list comparison to functions?  These are in package
> collection_helpers, but they are being moved to a different package currently.

I made a simple one in utils.dart (this bugged me a bit as well).

https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_runner.dart:175:
quotedArguments.addAll(arguments.map(escapeCommandLineArgument));
On 2013/12/19 11:00:00, Bill Hesse wrote:
> return ([escapeCommandLineArgument(executable)]
>     ..addAll(arguments.map(escapeCommandLineArgument)))
>     .join(' ');
> 
> ?

How about: 
return ([executable]..addAll(arguments))
        .map(escapeCommandLineArgument).join(' ');

https://codereview.chromium.org/118783003/diff/20001/dart/tools/testing/dart/...
dart/tools/testing/dart/test_runner.dart:280: if (env != null) {
On 2013/12/19 11:00:00, Bill Hesse wrote:
> In a different CL, this should be fixed.  It seems wrong that env is only
> created, and forwarding print set, only if additional dart flags are added.

Agreed.

kustermann

7 years ago (2013-12-19 13:02:36 UTC) #5

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages