Add Schema for Calibanconfig#37
Conversation
Codecov Report
@@ Coverage Diff @@
## master #37 +/- ##
==========================================
+ Coverage 48.29% 49.30% +1.00%
==========================================
Files 27 28 +1
Lines 2849 2872 +23
==========================================
+ Hits 1376 1416 +40
+ Misses 1473 1456 -17
Continue to review full report at Codecov.
|
ajslone
left a comment
ajslone
left a comment
There was a problem hiding this comment.
LGTM, just a couple of comments. Schema seems like a good choice.
| @@ -263,11 +265,10 @@ def validate_experiment_config(items: ExpConf) -> ExpConf: | |||
|
|
|||
|
|
|||
| def load_experiment_config(s): | |||
There was a problem hiding this comment.
Typing annotations would be good here, just given the ambiguity of the argument.
| return list(it.chain.from_iterable(pairs)) | ||
|
|
||
|
|
||
| def argparse_schema(schema): |
There was a problem hiding this comment.
Typing annotations would be helpful for me here, as I'm new to schema.
Would this be something along the lines of: (?)
SchemaType = Union[s.And, s.Or, s.Regex, s.Use, s.Schema, s.Const]
def argparse_schema(schema: SchemaType) -> Callable[Any, [Any]]:
| super().__init__(self.message) | ||
|
|
||
|
|
||
| @contextmanager |
There was a problem hiding this comment.
Great, context managers are very nice.
| # TODO Once a release with this patch happens: | ||
| # https://github.com/keleshev/schema/pull/238,, Change `Or` to `Schema`. This | ||
| # problem only occurs for callable validators. | ||
|
|
There was a problem hiding this comment.
The composition here is quite nice.
2 participants
This PR:
argparse-targeted validators with Schema versionsWhy?
The goal is to make it simpler to add new entries into calibanconfig. We can now share validators for calibanconfig entries with the validators we use for argparsing by wrapping the schemas in
ua.argparse_schema... and they will Just Work as argument validators.This makes #20 , #10 and other PRs like it nice and simple.