Update parser to match language as defined in README.md #25

gunnarx · 2022-04-27T08:15:38Z

First change to update parser to match current description of the VSC
syntax (as stated in README.md of vehicle_service_catalog, currently)

(There could still be some bugs in either this code, and/or the description in the README, but this brings the tools at least roughly up to date with what is described there).

erikbosch · 2022-04-27T12:40:30Z

model/vsc_parser.py

@@ -289,7 +308,9 @@ def ast_Arguments(parent, yamltree, argtype = 'in_arguments') -> list[Argument]:
    for st in subtrees:
        node = Argument(get_yaml_value(st, 'name'), parent)
        node.description = get_yaml_value(st, 'description')
-        node.type = get_yaml_value(st, 'datatype')
+        node.datatype = get_yaml_value(st, 'datatype')


We should preferably check how this change affect existing templates. For example, we have a regression for https://github.com/COVESA/vsc-tools/blob/master/templates/simple_overview.tpl as it refers to {{x.type}}. Have not checked output of other templates if there are any more regressions.

(With this PR we can later extend templates to also consider error information, but for now it is enough that we do not introduce any regressions)

We should preferably check how this change affect existing templates

Yes. The "existing templates" in /templates are controlled by the same code project and the intention was to update them accordingly, of course. In other words, it's a bug that the change to the template was not included in the push.

Daniel Wilms and have done some prototyping for a test framework for VSS to detect regressions (see e.g. COVESA/vss-tools#159, Daniel has been working on a similar concept using Python). Would be great if we later could add something similar so VSC-tools so regressions to templates would be detected by CI.

Agree, that could be useful, probably in combination with traditional unit tests based on input + expected output!
I would probably build out some more traditional tests first and add this "diffing against previous output" when there are more feature-full templates, and things have stabilized a bit more.

gunnarx · 2022-05-09T18:27:47Z

This is now more for reference because I imagine #26 will be the preferred implementation(?) In any case, I'd probably merge this first because it is relevant history.

erikbosch · 2022-05-10T06:56:31Z

model/vsc_parser.py

@@ -371,28 +412,44 @@ def ast_Properties(parent, yamltree) -> list[Property]:
    for st in subtrees:
        node = Property(get_yaml_value(st, 'name'), parent)
        node.description = get_yaml_value(st, 'description')
-        node.type = get_yaml_value(st, 'type')


I notice that "type" (i.e. sensor/attribute (and actuator?)) has been removed from the vsc README for properties, but is still used within the VSC seat example. If that is intentional we shall better remove it from the example.

(Or do we need to have something similar to attribute/sensor/actuator for properties in VSC, if not using those words maybe something like r/rw. We say in readme that "Each properties list object specifies a shared state object that can be read and set, and which is available to all subscribing entities.", so it seems that properties shall be writeable)

erikbosch · 2022-05-10T06:58:39Z

model/vsc_parser.py

@@ -305,6 +326,7 @@ def ast_Members(parent, yamltree, argtype = 'members') -> list[Member]:
        node = Member(get_yaml_value(st, 'name'), parent)
        node.description = get_yaml_value(st, 'description')
        node.type = get_yaml_value(st, 'datatype')


Shall also be changed to datatype?

I changed this and squashed it into the original commit and force pushed.

erikbosch · 2022-05-10T07:00:11Z

This is now more for reference because I imagine #26 will be the preferred implementation(?) In any case, I'd probably merge this first because it is relevant history.

I would prefer that the regressions in at least simple_overview.tpl are fixed before merge.

gunnarx · 2022-06-13T17:10:02Z

This is now more for reference because I imagine #26 will be the preferred implementation(?) In any case, I'd probably merge this first because it is relevant history.

I would prefer that the regressions in at least simple_overview.tpl are fixed before merge.

Fixed type->datatype in 3e419de and it seems to be generated now.

gunnarx · 2022-06-13T17:12:41Z

I think this is ready to be merged as an intermediate step, but #26 (major rewrite) would also be rebased on top of it. Either way you look at it, we would keep this step as a part of the history, as discussed above.

gunnarx · 2022-06-13T18:14:37Z

Since #28 was merged first, I had to rebase this. @waltersve, could you please check that the changes in 9751408 seem to be correct since in the meantime I had already (in 3e419de) modified type to datatype based on @erikbosch input above.

gunnarx · 2022-06-13T18:29:59Z

We should also get more testing up and running, hopefully.

The (very rudimentary) unit test failed because major/minor version were included in the BAMM code and are now mandatory, but were missing in the test input. Added fix above. Now it's successful.

erikbosch · 2022-06-14T06:24:29Z

templates/sds-bamm-macros.tpl

@@ -176,8 +174,8 @@ a bamm-c:RangeConstraint ;
 {{ member.name }}
 {%- elif member.datatype and member.datatype.endswith("_t") -%}
 Characteristic{{ snake_to_camel(member.datatype.capitalize()) }}
-{%- elif member.type and member.type.endswith("_t") -%}
-Characteristic{{ snake_to_camel(member.type.capitalize()) }}
+{%- elif member.datatype and member.datatype.endswith("_t") -%}


The lines 177-178 can actually be removed now when it is unified that "datatype" always is "datatype" (previously it was called "type" for attributes") so the workaround here can be removed

Ok. Will do. Thanks for the hint.

Yes, that was what I intended and I removed the if-conditions in a few places, but may have missed some. I didn't try to understand your logic much before, but I guess now that you were using _t suffix as a condition to see if it was a data type or an attribute?

erikbosch · 2022-06-14T06:25:44Z

templates/sds-bamm-macros.tpl

@@ -188,6 +186,6 @@ Characteristic{{ snake_to_camel(member.name.capitalize()) }}
 {%- if member.datatype -%}
 :{{ snake_to_camel(member.datatype.capitalize()) }}
 {%- else -%}
-:{{ snake_to_camel(member.type.capitalize()) }}
+:{{ snake_to_camel(member.datatype.capitalize()) }}


And here I assume line 186-188+190 can be removed

Signed-off-by: Gunnar Andersson <gunnar@[email protected]>

First change to update parser to match current description of the VSC syntax (as stated in README.md of vehicle_service_catalog, _currently_) (There could still be some bugs in either this code, and/or the description in the README)

This is to match the variable name in the code now. It might be necessary to do another update of templates in combination with the next iteration of the parser, a.k.a. "data driven" parser.

The changeset that added BAMM output generator included major-version and minor-version, that were missing from the parser. They were included as mandatory fields, so the rudimentary test failed. (I would have squashed this change into the same changeset but they were already merged)

gunnarx · 2022-06-20T10:33:15Z

OK, template sds-bamm-macros.tpl is now updated with Erik's help, and the PR cleaned up and rebased on master -> ready to merge.

erikbosch · 2022-06-20T17:12:42Z

Meeting decision to merge

erikbosch reviewed Apr 27, 2022

View reviewed changes

erikbosch reviewed May 10, 2022

View reviewed changes

gunnarx force-pushed the update_parser branch from d207952 to 3e419de Compare June 13, 2022 17:08

gunnarx force-pushed the update_parser branch from 3e419de to 9751408 Compare June 13, 2022 18:09

gunnarx mentioned this pull request Jun 13, 2022

Data driven parser #26

Merged

erikbosch reviewed Jun 14, 2022

View reviewed changes

gunnarx mentioned this pull request Jun 20, 2022

Introducing DTDL and Protobuf templates #27

Merged

gunnarx force-pushed the update_parser branch from eac26a6 to 5dd8901 Compare June 20, 2022 10:22

Gunnar Andersson and others added 4 commits June 20, 2022 12:25

parser: Whitespace and comments update

71a992c

Signed-off-by: Gunnar Andersson <gunnar@[email protected]>

parser: Update to match syntax in README.md

9820e85

First change to update parser to match current description of the VSC syntax (as stated in README.md of vehicle_service_catalog, _currently_) (There could still be some bugs in either this code, and/or the description in the README)

Templates: Update x.type to x.datatype

67c589f

This is to match the variable name in the code now. It might be necessary to do another update of templates in combination with the next iteration of the parser, a.k.a. "data driven" parser.

gunnarx force-pushed the update_parser branch from 5dd8901 to 30d5a5e Compare June 20, 2022 10:26

gunnarx merged commit ddd2895 into COVESA:master Jun 22, 2022

gunnarx deleted the update_parser branch August 4, 2023 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update parser to match language as defined in README.md #25

Update parser to match language as defined in README.md #25

gunnarx commented Apr 27, 2022

erikbosch Apr 27, 2022

gunnarx May 2, 2022

erikbosch May 9, 2022

gunnarx May 9, 2022

gunnarx commented May 9, 2022

erikbosch May 10, 2022

erikbosch May 10, 2022

gunnarx Jun 13, 2022

erikbosch commented May 10, 2022

gunnarx commented Jun 13, 2022

gunnarx commented Jun 13, 2022

gunnarx commented Jun 13, 2022

gunnarx commented Jun 13, 2022

erikbosch Jun 14, 2022

waltersve Jun 14, 2022

gunnarx Jun 20, 2022

erikbosch Jun 14, 2022

gunnarx commented Jun 20, 2022

erikbosch commented Jun 20, 2022

Update parser to match language as defined in README.md #25

Update parser to match language as defined in README.md #25

Conversation

gunnarx commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunnarx commented May 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikbosch commented May 10, 2022

gunnarx commented Jun 13, 2022

gunnarx commented Jun 13, 2022

gunnarx commented Jun 13, 2022

gunnarx commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunnarx commented Jun 20, 2022

erikbosch commented Jun 20, 2022