Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkgs/cli_config/analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
include: package:lints/recommended.yaml
include: package:dart_flutter_team_lints/analysis_options.yaml

analyzer:
language:
Expand Down
2 changes: 1 addition & 1 deletion pkgs/cli_config/lib/cli_config.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
/// and environment variables.
library cli_config;

export 'src/cli_config.dart';
export 'src/config.dart';
5 changes: 0 additions & 5 deletions pkgs/cli_config/lib/src/cli_config.dart

This file was deleted.

52 changes: 52 additions & 0 deletions pkgs/cli_config/lib/src/cli_parser.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'package:args/args.dart';

class CliParser {
final ArgParser parser = () {
final parser = ArgParser();
parser.addFlag(
'help',
abbr: 'h',
help: 'Show this help.',
);
parser.addMultiOption(
'define',
abbr: 'D',
help: '''Define or override a config property from command line.
The same option can be passed multiple times.
Keys should only contain lower-case alphanumeric characters, underscores,
and '.'s''',
);
parser.addOption(
'config',
abbr: 'c',
help: '''Path to JSON or YAML config file.
Keys should only contain lower-case alphanumeric characters, and underscores.
Hierarchies should be maps.''',
);
return parser;
}();

ArgResults parse(List<String> args) => parser.parse(args);
}

class DefinesParser {
Map<String, List<String>> parse(List<String> args) {
final regex = RegExp('([a-z_.]+)=(.+)');
final defines = <String, List<String>>{};
for (final arg in args) {
final match = regex.matchAsPrefix(arg);
if (match == null || match.group(0) != arg) {
throw FormatException("Define '$arg' does not match expected pattern "
"'${regex.pattern}'.");
}
final key = match.group(1)!;
final value = match.group(2)!;
defines[key] = (defines[key] ?? [])..add(value);
}
return defines;
}
}
84 changes: 84 additions & 0 deletions pkgs/cli_config/lib/src/cli_provider.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'config.dart';
import 'provider.dart';

class CliProvider extends Provider {
/// Configuration options passed in via CLI arguments.
///
/// Options can be passed multiple times, so the values here are a list.
///
/// Stored as a flat non-hierarchical structure, keys contain `.`.
final Map<String, List<String>> _cli;

CliProvider(this._cli);

@override
String? getOptionalString(String key) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, config.string('key') vs config.readString('key') or config.loadString('key'). I guess we don't care about the verb so much.

Unfortunately, config.bool('key') doesn't work, because bool is a keyword.

config.stringValue('key') seems more bloated than using a verb.

get is the shortest verb.

Copy link
Member

@HosseinYousefi HosseinYousefi Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like get because this looks essentially like a map.

An alternative solution is to create a CliResult object that operator [] returns. Then config.getOptionalString('key') becomes config['key'].as<String>(), .as<bool>() and ...

.as<T>() for an unsupported T throws a FormatException.

Edit: Oops, as is also a keyword! maybe .value<String>()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API here is similar to shared_preferences. And there we have getString and getStringList as well.

Copy link
Contributor

@jakemac53 jakemac53 Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use read, or consider dropping the entire verb and using boolean instead of bool? read is only one more character.

I don't ultimately feel super strongly for this particular case, but I do care that Dart owned/maintained packages follow effective Dart guidelines, so we don't set an example that is contrary to effective Dart. Ultimately people will imitate the types of apis we put out there, and use them as justification for their own, unaware of any deeper reasoning. In fact shared_preferences is a perfect example of this, in this exact thread...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/flutter/packages/blame/dd8b54101c056190d15e5a1f6ad2b491657f23c2/packages/shared_preferences/lib/shared_preferences.dart I don't seen any code review for our shared_preferences initial PR that introduced this API.

Since read is a verb that communicates not that much here (also, technically speaking, part or the reading has happened already when reading in the various sources), it is a good argument for not having the verb. And being in the case that it should be a getter even though it has is a method with arguments.

The issue here is that the no-verb solution conflicts with keywords. And choosing boolean, integer, and doublee, would create confusion because they are spelled the same as the type they return.

@munificent touched this part of the style guide last, any neat ideas of what to do with conflicts with keywords?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd drop the verb. Changing get to read, which still has no semantic meaning different from "get", "fetch", "lookup" or "find", is not an improvement.

Don't use words that say nothing. Document and name getter-like methods like get getters (noun phrases).
That's what this is.

I'd use string, stringValue, stringValueOf or (more dubious) stringOf.

For bool and int, where we may don't want to get naming conflicts (we can, they are not reserved words, they just shadow the core types), we can use boolean and integer, or boolValue/intValue.

Or just use int and bool and deal with the conflicts:

import "dart:core";
import "dart:core" as core;

....

   core.bool bool(String key) { ... }

(You need to use core.bool everywhere in the class, highly annoying.)

final value = _cli[key];
if (value == null) {
return null;
}
if (value.length > 1) {
throw FormatException(
"More than one value was passed for '$key' in the CLI defines."
' Values passed: $value');
}
return value.single;
}

@override
List<String>? getOptionalStringList(
String key, {
String? splitPattern,
}) {
final cliValue = _cli[key];
if (cliValue == null) {
return null;
}
if (splitPattern != null) {
return [for (final value in cliValue) ...value.split(splitPattern)];
}
return cliValue;
}

@override
bool? getOptionalBool(String key) {
final stringValue = getOptionalString(key);
if (stringValue != null) {
Provider.throwIfUnexpectedValue(
key, stringValue, Config.boolStrings.keys);
return Config.boolStrings[stringValue]!;
}
return null;
}

@override
Uri? getOptionalPath(
String key, {
bool resolveUri = false,
}) {
assert(resolveUri == false);
final stringValue = getOptionalString(key);
if (stringValue != null) {
return Provider.fileSystemPathToUri(stringValue);
}
return null;
}

@override
List<Uri>? getOptionalPathList(
String key, {
String? splitPattern,
bool resolveUri = false,
}) {
assert(resolveUri == false);
final strings = getOptionalStringList(key, splitPattern: splitPattern);
return strings?.map((e) => Uri(path: e)).toList();
}

@override
String toString() => 'CliProvider($_cli)';
}
Loading