fix: array decoding in sysctl-runner#775
Conversation
alexruzenhack
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #775 +/- ##
==========================================
+ Coverage 84.66% 84.69% +0.03%
==========================================
Files 264 260 -4
Lines 22034 21955 -79
Branches 3369 2966 -403
==========================================
- Hits 18654 18594 -60
+ Misses 2725 2712 -13
+ Partials 655 649 -6
☔ View full report in Codecov by Sentry. |
hathor/sysctl/runner.py
Outdated
| # - It ends with a closing square bracket ]. | ||
| # - The elements are separated by commas and can be followed by optional whitespace. | ||
| # - There can be zero or more elements in the array (an empty array is allowed). | ||
| array_pattern = r'^\s*\[\s*(?:[^\[\],]+(?:\s*,\s*[^\[\],]+)*)?\s*\]\s*$' |
There was a problem hiding this comment.
The regex pattern is hard to understand for me. I understand the "positive" cases, but what are the "negative" cases: what are we trying to prevent from being parsed as a list/array by using this regex?
There was a problem hiding this comment.
I've update it and tried to make the regex clearer.
There was a problem hiding this comment.
I've seen it, the updated one is even longer. I used some tools to break it down and understand how it works. However I don't like the solution too much, because we're making a regex pattern that really has to understand JSON syntax, and I don't think it's really possible with regex. I have a different proposal to tackle this without having to use a regex I'll explain further on our chat with @msbrogli so we can discuss it better.
| return () | ||
|
|
||
| if re.match(array_pattern, value_str): | ||
| return list(json.loads(value_str)) |
There was a problem hiding this comment.
Won't this produce a list inside a list? I don't understand how this is working.
There was a problem hiding this comment.
No. The list(...) is consolidating to list type, apparently. Take a look:
There was a problem hiding this comment.
I guess this solution would not work if the method returns a tuple[int, list[str]], for instance. But I haven't really tested against this case yet.
alexruzenhack
force-pushed
the
feat/add-optional-argument
branch
2 times, most recently
from
7404f94 to
f6edfcf
Compare
This reverts commit aadaf75.
- allow list to parse number and strings in double quote - don't allow empty elements and comma after last element
alexruzenhack
force-pushed
the
fix/sysctl-runner
branch
from
4336d8d to
7b43158
Compare
|
Replaced by #820 |
3 participants
Motivation
Fix SysctlRunner array decode.
Acceptance Criteria
test_command_option_sysctl_init_fileintests/cli/test_sysctl_init.py.Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged