Allow builtins and resolved paths as cli types #352
Conversation
|
An essential problem with the current fix: with snakemake, every CLI arg will have to be serialized into YAML. Even with paths, we don't actually keep them as paths, they get turned into strings. So I'm going to add checks to ensure that all types selected can be simply serializable into primitive yaml types. I'll keep the module path logic though, as I already have the tests, and it may be useful in a more generic future if snakebids works with tools beyond snakemake |
pvandyken
force-pushed
the
feat/builtins-as-types
branch
from
ba3f546 to
90b0845
Compare
| @@ -68,6 +67,7 @@ copier = ">=8.2.0" | |||
| jinja2-time = ">=0.2.0" | |||
| # minimum 2.31.0 because of security vulnerability | |||
| requests = ">=2.31.0" | |||
| ruamel-yaml = ">=0.17.2" | |||
There was a problem hiding this comment.
Switched to ruamel because pyyaml didn't support allowing only specified types to be serialized, it either tried to serialize everything (using weird custom python tags that snakemake would then refuse to read), or only serialize basic types (but we need it to specifically support Path). ruamel did support this.
If we switch to reading yaml with ruamel in the future, we could further take advantage of its round-trip comment preservation
| @@ -204,6 +205,26 @@ def create_parser(include_snakemake: bool = False) -> argparse.ArgumentParser: | |||
| return parser | |||
|
|
|||
|
|
|||
| def _find_type(name: str, *, yamlsafe: bool = True) -> type[Any]: | |||
There was a problem hiding this comment.
yamlsafe is unused, but leaving this more as a hint to the future that we may want to support more flexible parsing of types for different applications
pvandyken
force-pushed
the
feat/builtins-as-types
branch
from
38626c3 to
48e3bfe
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
+ Coverage 89.14% 90.26% +1.11%
==========================================
Files 30 32 +2
Lines 1465 1489 +24
==========================================
+ Hits 1306 1344 +38
+ Misses 159 145 -14 ☔ View full report in Codecov by Sentry. |
|
I left a few specific comments above. I'll write a test for the config writing function (although this functionality wasn't actually tested before), but should be good for review nonetheless |
pvandyken
force-pushed
the
feat/builtins-as-types
branch
3 times, most recently
from
f98f97d to
363bbd3
Compare
The architecture for type conversion in the argparse integration was intended for using pathlib.Path, but had no flexibility even for builtin types like int Here, remove the old `globals()` lookup, which exposed too much internal api. Instead, attempt to lookup simple strings as builtins, and strings with dots as resolved module imports (e.g. `module.submodule.type`). `Path` is kept as an exception, so using `type: Path` will still convert the argument into a path ruamel is now used for serialization, as pyyaml didn't support serialization of specific python classes (e.g. Path) without restricting other classes: it was all or nothing. ruamel is safe by default, and allows specifically opting in to representing different classes Clarify in docs how Path should be used for all paths to avoid relative path issues Resolves khanlab#294
ruamel fails to roundtrip the \x85 unicode character
pvandyken
force-pushed
the
feat/builtins-as-types
branch
from
363bbd3 to
762d37c
Compare
The architecture for type conversion in the argparse integration was intended for using pathlib.Path, but had no flexibility even for builtin types like int
Here, remove the old
globals()lookup, which exposed too much internal api. Instead, attempt to lookup simple strings as builtins, and strings with dots as resolved module imports (e.g.module.submodule.type).Pathis kept as an exception, so usingtype: Pathwill still convert the argument into a pathResolves #294