-
Notifications
You must be signed in to change notification settings - Fork 77
[cli_config] First version #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
414a8fb
[cli_config] First version
dcharkes 7ec934d
Address comments
dcharkes af82a32
Address comments
dcharkes 07ca71b
Rename `Provider` to `Source`
dcharkes 8244bbe
Introduce `valueOf` for all sources
dcharkes 0c43865
Drop `get` verb from API
dcharkes 95c1bc8
Fix doc comment
dcharkes a1e1251
Use current working directory as baseUri for command line paths
dcharkes b2361de
Add `int` config values
dcharkes 5702466
Add `double` config values
dcharkes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,4 +6,4 @@ | |
| /// and environment variables. | ||
| library cli_config; | ||
|
|
||
| export 'src/cli_config.dart'; | ||
| export 'src/config.dart'; | ||
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| // 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 { | ||
| static final _defineRegex = RegExp('([a-z_.]+)=(.+)'); | ||
|
|
||
| Map<String, List<String>> parse(List<String> args) { | ||
| final defines = <String, List<String>>{}; | ||
| for (final arg in args) { | ||
| final match = _defineRegex.matchAsPrefix(arg); | ||
| if (match == null || match.group(0) != arg) { | ||
| throw FormatException("Define '$arg' does not match expected pattern " | ||
| "'${_defineRegex.pattern}'."); | ||
| } | ||
| final key = match.group(1)!; | ||
| final value = match.group(2)!; | ||
| defines[key] = (defines[key] ?? [])..add(value); | ||
| } | ||
| return defines; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| // 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) { | ||
| 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 | ||
| String toString() => 'CliProvider($_cli)'; | ||
|
|
||
| @override | ||
|
|
||
| /// CLI paths are not resolved. | ||
| Uri? get baseUri => null; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://dart.dev/guides/language/effective-dart/design#avoid-starting-a-method-name-with-get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm,
config.string('key')vsconfig.readString('key')orconfig.loadString('key'). I guess we don't care about the verb so much.Unfortunately,
config.bool('key')doesn't work, becauseboolis a keyword.config.stringValue('key')seems more bloated than using a verb.getis the shortest verb.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like
getbecause this looks essentially like a map.An alternative solution is to create a
CliResultobject thatoperator []returns. Thenconfig.getOptionalString('key')becomesconfig['key'].as<String>(),.as<bool>()and ....as<T>()for an unsupportedTthrows aFormatException.Edit: Oops,
asis also a keyword! maybe.value<String>()?There was a problem hiding this comment.
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 havegetStringandgetStringListas well.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 usingbooleaninstead ofbool?readis 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_preferencesis a perfect example of this, in this exact thread...There was a problem hiding this comment.
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
readis 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, anddoublee, 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?
There was a problem hiding this comment.
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
gettoread, 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,stringValueOfor (more dubious)stringOf.For
boolandint, 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 usebooleanandinteger, orboolValue/intValue.Or just use
intandbooland deal with the conflicts:(You need to use
core.booleverywhere in the class, highly annoying.)