-
Notifications
You must be signed in to change notification settings - Fork 68
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
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.
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing annotations would be good here, just given the ambiguity of the argument.
@@ -39,6 +40,25 @@ def expand_args(items: Dict[str, str]) -> List[str]: | |||
return list(it.chain.from_iterable(pairs)) | |||
|
|||
|
|||
def argparse_schema(schema): |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The composition here is quite nice.
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.