params: Use ast.literal_eval for python params.
#6954
Conversation
daavoo
force-pushed
the
use-literal-eval-python-params
branch
from
71a4deb to
850282c
Compare
|
@aandrusenko @skshetry (from #4456) would you mind taking a look, please? |
|
I have no comments |
| @@ -0,0 +1,45 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
The code part LGTM.
ast.literal_eval are quite stable. I don't consider it to be worthy to do a detailed test on it. But here the tests involve collections and sequences. I just wondered that we don't test them in previous?
There was a problem hiding this comment.
We test them here:
https://github.com/iterative/dvc/blob/master/tests/unit/dependency/test_params.py#L114
I went for a separate unit test as that one involves ParamsDependency and Stage in the mix.
I could remove this file and just add a test case for the UNARY_OP in the file linked above.
There was a problem hiding this comment.
I don't consider it to be worthy to do a detailed test on it.
It's not testing ast.literal_eval, it's testing that we are converting assignment statement into a key-value pair correctly.
There was a problem hiding this comment.
I don't consider it to be worthy to do a detailed test on it.
It's not testing
ast.literal_eval, it's testing that we are converting assignment statement into a key-value pair correctly.
Yes, I know that But here the tests involve collections and sequences. I just wondered that we don't test them in previous?
|
@Mergifyio rebase |
Add unit tests for `parse_py` Closes #6953
✅ Branch has been successfully rebased |
skshetry
force-pushed
the
use-literal-eval-python-params
branch
from
850282c to
43b46d4
Compare
Add unit tests for
parse_pyCloses #6953
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
dvc.orgP.R: iterative/dvc.org#3014