Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kuksa Python & Go client: Refactor array handling and add tests #596

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

erikbosch
Copy link
Contributor

@erikbosch erikbosch commented Jul 10, 2023

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.

@erikbosch erikbosch force-pushed the erik_array branch 2 times, most recently from b319388 to 3308d93 Compare July 10, 2023 11:52
def cast_array_values(cast, array):

array = array.strip('[]')
pattern = r'(?:\\"|[^",])+|"(?:\\"|[^"])*"'
Copy link
Contributor

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.

Copy link
Contributor Author

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"

Copy link
Contributor Author

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"
    }
}

@@ -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 := `(?:\\"|[^",])+|"(?:\\"|[^"])*"|'(?:\\"|[^']|\\')*'`
Copy link
Contributor

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"
    }
}

Copy link
Contributor Author

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.

Copy link
Contributor Author

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"
    }
}

@erikbosch erikbosch changed the title Refactor array handling and add tests Kuksa Python & Go client: Refactor array handling and add tests Jul 11, 2023
@erikbosch erikbosch force-pushed the erik_array branch 9 times, most recently from 42118a7 to b7321d1 Compare July 12, 2023 07:06
@erikbosch erikbosch marked this pull request as ready for review July 12, 2023 15:21
Copy link
Contributor

@lukasmittag lukasmittag left a 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.

@erikbosch erikbosch merged commit 7937c61 into eclipse:master Jul 13, 2023
7 checks passed
@erikbosch erikbosch deleted the erik_array branch July 13, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants