-
Notifications
You must be signed in to change notification settings - Fork 51
Kuksa Python & Go client: Refactor array handling and add tests #596
Conversation
b319388
to
3308d93
Compare
def cast_array_values(cast, array): | ||
|
||
array = array.strip('[]') | ||
pattern = r'(?:\\"|[^",])+|"(?:\\"|[^"])*"' |
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.
This will lead from
setValue Vehicle.OBD.DTCList ["dtc1, dtc2", dtc3, \" dtc4, dtc4\"]
to
{
"path": "Vehicle.OBD.DTCList",
"value": {
"value": {
"values": [
"dtc1, dtc2",
"dtc3",
"\" dtc4",
"dtc4\""
]
},
"timestamp": "2023-07-11T06:37:13.441133+00:00"
}
}
the comma in " will not be treated correct.
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.
In my view the interpretation seems to be correct, the escaped quote is treated as belonging to the string rather than marking an item. Or do you want that the following two strings should give the same result?
["dtc1, dtc2", dtc3, \" dtc4, dtc4\"]
["dtc1, dtc2", dtc3, " dtc4, dtc4"]
I see a semantic difference, the first consisting of 4 fields where the third field is " dtc4
and the fourth dtc4"
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.
I am adding some recommendations to the README. Main problem is that not surrounding the whole array with quotes leaves you to the special handling of the shell/argparse. Like in this case (blank before first double quote)
setValue Vehicle.OBD.DTCList [ "dtc1, dtc2", dtc3, \" dtc4, dtc4\"]
... which neither now or before give you what you want as the shell "eats" the quotes. We could possibly do something about it, like add quotes back but that gives us a lot of corner cases where we want to escape any quotes inside, but not quotes that already are escaped. So for now we need to live with that limitation. Better then to use surround everything with single quotes:
setValue Vehicle.OBD.DTCList '[ "dtc1, dtc2", dtc3, \" dtc4, dtc4\"]'
Which gives you what i think is correct
Test Client> getValue Vehicle.OBD.DTCList
{
"path": "Vehicle.OBD.DTCList",
"value": {
"value": {
"values": [
"dtc1, dtc2",
"dtc3",
"\" dtc4",
"dtc4\""
]
},
"timestamp": "2023-07-12T06:19:14.369683+00:00"
}
}
kuksa_go_client/kuksa_client/grpc.go
Outdated
@@ -83,7 +83,7 @@ func getArrayFromInput[T any](input string) ([]T, error) { | |||
input = strings.TrimSuffix(strings.TrimPrefix(input, "["), "]") | |||
|
|||
// Split the input string into separate values | |||
pattern := `(?:\\.|[^",])*"(?:\\.|[^"])*"|'(?:\\.|[^']|\\')*'|[^",]+` | |||
pattern := `(?:\\"|[^",])+|"(?:\\"|[^"])*"|'(?:\\"|[^']|\\')*'` |
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.
In GOit is only possible to set a certain value through for example
"['dtc1, dtc2', dtc2, \"dtc3, dtc3\"]"
and this will result in
{
"path": "Vehicle.OBD.DTCList",
"value": {
"value": {
"values": [
"dtc1",
"dtc2",
"dtc2",
"",
"\"dtc3, dtc3\""
]
},
"timestamp": "2023-07-11T06:41:14.537216+00:00"
}
}
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.
This seems odd, I will need to look into it a bit more. But then it is also so that it is a difference between how a string is set in code on what it actually will contain.
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.
With latest PR version the string /from main.go)
var valstr = "['dtc1, dtc2', dtc2, \"dtc3, dtc3\", dtc4]"
Results in:
Test Client> getValue Vehicle.OBD.DTCList
{
"path": "Vehicle.OBD.DTCList",
"value": {
"value": {
"values": [
"dtc1, dtc2",
"dtc2",
"dtc3, dtc3",
"dtc4"
]
},
"timestamp": "2023-07-12T06:11:15.227045+00:00"
}
}
42118a7
to
b7321d1
Compare
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.
Didn't manage to run the tests. Maybe related to python 3.8 or something else. Also error is in conftest.py not newly added one. Other than that I did run go tests and go client looked good.
kuksa-client and README tested and tested a uint8Array works as well as expected.
This is work in progress. Background is that code scanning showed that the current regexp may be inefficient for certain strings. To see if it could be simplified without causing side effects a number of test cases has been added, which also partially proves as documentation on how it is intended to work. A similar change is one for the go client.