Server options handling.

Server options handling.

R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/88b8fb8e1efa9db19765c5d83bf8d3e25c3e6591

[pquitslund, 2015-07-30 17:49:19]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[pquitslund, 2015-07-30 17:49:19]

A jumping off point for server options handling.

[scheglov, 2015-07-30 18:04:10]

LGTM

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

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/server_options.dart:53: }
If the options file does not exist, we will fall-through here.
Maybe more the return statement from the catch block here?

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/test/se...
File pkg/analysis_server/test/server_options_test.dart (right):

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/server_options_test.dart:34:
expect(options.isSet('foobar'), isFalse);
Test for not existing option?

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/server_options_test.dart:35: });
Test for getStringValue()?

[Brian Wilkerson, 2015-07-30 18:18:59]

LGTM with only minor comments

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

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/server_options.dart:5: // This code was
auto-generated, is not intended to be edited, and is subject to
?

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/server_options.dart:8: library
analysis_server.server_options;
Should be "analysis_server.src.server_options".

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/server_options.dart:37: // Shouldn't happen but to
be safe.
This can happen if either (a) the platform is not known or (b) if the
environment variable is not set.

Neither of those is common, but neither is impossible. We should update the
comment to include that information (or add a doc comment).

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/server_options.dart:48: if
(optionsFile.existsSync()) {
Add "optionsFile != null &&"

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/server_options.dart:62: final Map<String, dynamic>
_options = <String, dynamic>{};
It probably doesn't matter much here, but we've adopted a style of always using
"new HashMap<...>()" rather than a literal map because it's more efficient.

https://codereview.chromium.org/1270593002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/server_options.dart:95: ///     myDebugOption2:on  
# Also read as `false`.
nit: it would be more consistent if either all the keys were the same or all the
keys were unique.

[pquitslund, 2015-07-30 19:55:48]

Thanks!

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
File pkg/analysis_server/lib/src/server_options.dart (right):

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
pkg/analysis_server/lib/src/server_options.dart:5: // This code was
auto-generated, is not intended to be edited, and is subject to
On 2015/07/30 18:18:59, Brian Wilkerson wrote:
> ?

Done.

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
pkg/analysis_server/lib/src/server_options.dart:8: library
analysis_server.server_options;
On 2015/07/30 18:18:59, Brian Wilkerson wrote:
> Should be "analysis_server.src.server_options".

Done.

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
pkg/analysis_server/lib/src/server_options.dart:37: // Shouldn't happen but to
be safe.
On 2015/07/30 18:18:59, Brian Wilkerson wrote:
> This can happen if either (a) the platform is not known or (b) if the
> environment variable is not set.
> 
> Neither of those is common, but neither is impossible. We should update the
> comment to include that information (or add a doc comment).

Done.

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
pkg/analysis_server/lib/src/server_options.dart:48: if
(optionsFile.existsSync()) {
On 2015/07/30 18:18:59, Brian Wilkerson wrote:
> Add "optionsFile != null &&"

Done.

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
pkg/analysis_server/lib/src/server_options.dart:53: }
On 2015/07/30 18:04:10, scheglov wrote:
> If the options file does not exist, we will fall-through here.
> Maybe more the return statement from the catch block here?

Ah, yes.  Good catch!

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
pkg/analysis_server/lib/src/server_options.dart:62: final Map<String, dynamic>
_options = <String, dynamic>{};
On 2015/07/30 18:18:59, Brian Wilkerson wrote:
> It probably doesn't matter much here, but we've adopted a style of always
using
> "new HashMap<...>()" rather than a literal map because it's more efficient.

NP.  Updated.

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
pkg/analysis_server/lib/src/server_options.dart:95: ///     myDebugOption2:on  
# Also read as `false`.
On 2015/07/30 18:18:59, Brian Wilkerson wrote:
> nit: it would be more consistent if either all the keys were the same or all
the
> keys were unique.

Done.

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
File pkg/analysis_server/test/server_options_test.dart (right):

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
pkg/analysis_server/test/server_options_test.dart:34:
expect(options.isSet('foobar'), isFalse);
On 2015/07/30 18:04:10, scheglov wrote:
> Test for not existing option?

Done.

https://chromiumcodereview.appspot.com/1270593002/diff/1/pkg/analysis_server/...
pkg/analysis_server/test/server_options_test.dart:35: });
On 2015/07/30 18:04:10, scheglov wrote:
> Test for getStringValue()?

Done.

[pquitslund, 2015-07-30 19:59:04]

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