search

ReproNim / neurodocker

Generate custom Docker and Singularity images, and minimize existing containers
https://www.repronim.org/neurodocker/
Apache License 2.0
326 stars 97 forks source link

fix the types in --copy and --entrypoint #500

Closed kaczmarj closed 1 year ago

kaczmarj commented 1 year ago

Both of these options use the class OptionEatAll. By default, this class will output a string type, but that would result in a string that looked like a tuple. For example, it is possible to use ast.literal_eval(value) on the value to convert it back to a tuple. To fix this, this commit sets the type for --copy and --entrypoint to tuple and the subsequent code expects the values for --copy and --entrypoint to be tuples.

This commit also adds a type hint to an OptionsEatAll internal to help with debugging.

related to #498 #499

codecov[bot] commented 1 year ago

Codecov Report

Base: 83.67% // Head: 88.77% // Increases project coverage by +5.09% :tada:

Coverage data is based on head (30cb8d3) compared to base (95f8f91). Patch coverage: 84.21% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #500 +/- ## ========================================== + Coverage 83.67% 88.77% +5.09% ========================================== Files 11 11 Lines 1017 1033 +16 ========================================== + Hits 851 917 +66 + Misses 166 116 -50 ``` | [Impacted Files](https://codecov.io/gh/ReproNim/neurodocker/pull/500?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ReproNim) | Coverage Δ | | |---|---|---| | [neurodocker/cli/generate.py](https://codecov.io/gh/ReproNim/neurodocker/pull/500?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ReproNim#diff-bmV1cm9kb2NrZXIvY2xpL2dlbmVyYXRlLnB5) | `91.66% <75.00%> (+0.67%)` | :arrow_up: | | [neurodocker/cli/cli.py](https://codecov.io/gh/ReproNim/neurodocker/pull/500?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ReproNim#diff-bmV1cm9kb2NrZXIvY2xpL2NsaS5weQ==) | `90.00% <100.00%> (-10.00%)` | :arrow_down: | | [neurodocker/cli/minify/trace.py](https://codecov.io/gh/ReproNim/neurodocker/pull/500?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ReproNim#diff-bmV1cm9kb2NrZXIvY2xpL21pbmlmeS90cmFjZS5weQ==) | `83.90% <0.00%> (+58.62%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ReproNim). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ReproNim)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

kaczmarj commented 1 year ago

@Remi-Gau @satra - whenever you are ready, please take a look. good on my end.

kaczmarj commented 1 year ago

a better fix might be to implement the type_cast_value() instance method on OptionEatAll but what we have in this PR works... implementing type_cast_value() wouldn't be so trivial because there might be corner cases that will cause unexpected behavior. so this would require extensive testing. instead, i suggest we do not allow the use of string type in OptionEatAll.

see https://click.palletsprojects.com/en/8.1.x/api/#click.Parameter.type_cast_value