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

Data driven parser #26

Merged
merged 10 commits into from
Jun 22, 2022
Merged

Data driven parser #26

merged 10 commits into from
Jun 22, 2022

Conversation

gunnarx
Copy link
Collaborator

@gunnarx gunnarx commented May 9, 2022

This is a completely rewritten parser (and some resulting modifications to generator) that is more "schema-driven". The VSC language is defined with a data structure that is like a "schema" or language definition, and a generic engine is operating on that.

"in" : (list[Argument], OPT)
},

"Property" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment in other VSC PR. Is it aligned that properties always are read-write, i.e. no need to include any information concerning that? (The current VSC seat example include a field type that specifies attribute, likely inspired by VSS.

properties:
      - name: a_property # Will this be acceptable?
        description: A signal
        type: attribute # or sensor.
        datatype: uint8 # Native datatypes only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it aligned that properties always are read-write, i.e. no need to include any information concerning that?

I don't know but it's a question for the VSC definition itself. (Can you create an issue for this question?)

(The current VSC seat example include a field type that specifies attribute, likely inspired by VSS.

Yes I noticed the difference compared to the previous parser but decided to implement following the README (Properties) as closely as I could, since it acts as our specification at the moment. :

Also, if we do #1 it would warn about unexpected stuff appearing in the YAML (e.g. type), which the current parser is not designed to do yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gunnarx
Copy link
Collaborator Author

gunnarx commented Jun 13, 2022

Rebased on top of #83 (which is also up to date with current master)

@gunnarx
Copy link
Collaborator Author

gunnarx commented Jun 13, 2022

The tests ran on 3.8 and fail because the code now uses the match statement introduced in python 3.9
Would you all be OK with requiring >=3.9 for python for the vsc tools? @erikbosch @magnusfeuer @waltersve, Tim, Mikhail (don't know your @ usernames)

@gunnarx gunnarx marked this pull request as ready for review June 16, 2022 08:32
@gunnarx
Copy link
Collaborator Author

gunnarx commented Jun 16, 2022

Sorry, python 3.10 is where the match statement was introduced. My mistake. @erikbosch @waltersve @magnusfeuer @ mikahil

@gunnarx gunnarx force-pushed the data_driven_parser branch 2 times, most recently from 2fa44a7 to 2635ca8 Compare June 16, 2022 08:50
@erikbosch
Copy link
Contributor

For me dependencies to a newer Python is no problem as it just concerns tooling (and nothing that shall run inside the vehicle). But don't we need to update e.g. https://github.com/COVESA/vsc-tools/blob/master/Pipfile then as well to require 3.10?

I am in favor of merging this one even if not all parts are perfect, to reduce dependencies. I would however like us to briefly discuss the change of properties to remove sensor/attribute/actuator and always consider them as read/write. The change in this PR is aligned with the formal syntax for VSC but not the example - and some of the templates currently use the information. If we all agree that we do not need any information on whether a property is read-only or read-write I am fine. I just want to avoid that we remove it from the templates and one month later realize that we still need it.

@@ -12,4 +12,4 @@ pytest = ">=2.7.2"
[dev-packages]

[requires]
python_version = "3.8"
python_version = "3.10"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the lock file also needs to be updated?

@gunnarx gunnarx force-pushed the data_driven_parser branch 2 times, most recently from ca999df to 74ca588 Compare June 20, 2022 10:34
@gunnarx
Copy link
Collaborator Author

gunnarx commented Jun 20, 2022

Rebased on top of #83 to prepare for merge. (So until 25 is merged, it will temporarily appear as if this change proposal includes both 25 and 26 changes). Actual 26 changes start from a1b2636.

@erikbosch
Copy link
Contributor

Meeting decision: Ok to merge after pipfile.lock has been updated

Gunnar Andersson and others added 10 commits June 22, 2022 11:31
Fix inconsistent indentation first to prepare for upcoming changes.
Rewrite of significant core parts of the parser.  Instead of explicit
classes and explicit readers for each class in the AST, it now has a
generic reader function for any "Node", where the behavior is driven by
an input data-structure that acts as a schema definition.
Ensure nodes have the right parent node so that the AnyTree hierarchy
path is correct.  Although it didn't affect most generators, this means
AST can be navigated either through navigating the member variables of
the AST objects, or by using AnyTree methods to address/search/etc. in the
hierarchical path that AnyTree keeps track of.
The latest rewrite introduced language features from python 3.10
Adjust Pipfile / lock, and the GitHub actions test accordingly.
@gunnarx gunnarx merged commit ea2003d into COVESA:master Jun 22, 2022
@gunnarx gunnarx deleted the data_driven_parser branch July 9, 2022 16:37
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