COVESA / vss-tools

Software for working with VSS (https://github.com/COVESA/vehicle_signal_specification)
Mozilla Public License 2.0
55 stars 55 forks source link

🐛 Fixed null values from input vspecs being removed #397

Closed sschleemilch closed 1 month ago

sschleemilch commented 1 month ago

About

That commented case does not really apply to what mapping actually contains.

However, the code that manipulates the mapping is problematic.

Example:

Vehicle:
  type: branch
  description: Vehicle
  foo: ~
  bar: ~

Expected export result (of course with allowing those additional fields):

Vehicle:
  bar: null
  description: Vehicle
  foo: null
  type: branch

Output of current 4.2:

Vehicle:
  bar: null
  description: Vehicle
  type: branch
SebastianSchildt commented 1 month ago

I am not sure, I fully understand what is ifxed here, especially wrt to output. At least if you add an additional field MySpecialfield , and only add this to one of the 100 VSS leafs, in output (at least yaml, json) you would expect that to be present one time, and not have 999 "null"/"None" things.

But I am not sure this is, what this is about?

sschleemilch commented 1 month ago

I am not sure, I fully understand what is ifxed here, especially wrt to output. At least if you add an additional field MySpecialfield , and only add this to one of the 100 VSS leafs, in output (at least yaml, json) you would expect that to be present one time, and not have 999 "null"/"None" things.

But I am not sure this is, what this is about?

It is about the output eating up attributes caused by the removed code in this PR. The output should also have foo as None

SebastianSchildt commented 1 month ago

Note to self (and others whom it may concern) ~ is a valid way to represent null in YAML. So I learned something today

SebastianSchildt commented 1 month ago

Meeting: 08/27: Please review. Merge if we get an approval.

SebastianSchildt commented 1 month ago

If this goes into the "4.x" breanch waht is the correct merge target @erikbosch

erikbosch commented 1 month ago

I have tried to explain the current approach at https://github.com/COVESA/vehicle_signal_specification/wiki/VSS-Version-Governance-(Major-Minor)#general-branch-strategy:

So the short term question is - does @sschleemilch or someone else wants a 4.2.1 patch or a 4.3 release?

sschleemilch commented 1 month ago

I have tried to explain the current approach at https://github.com/COVESA/vehicle_signal_specification/wiki/VSS-Version-Governance-(Major-Minor)#general-branch-strategy:

  • A PR towards 4.X would be appropriate if we want this to be included in the next 4.X release (if any), i.e. 4.3.
  • A PR towards release/4.2 would be appropriate if we want to create a 4.2.1 patch release

So the short term question is - does @sschleemilch or someone else wants a 4.2.1 patch or a 4.3 release?

Since this is a bugfix I would say there should be a 4.2.1 here

erikbosch commented 1 month ago

I merged it and added a point to the call tomorrow if we shall create a patch release and if so what artifacts that are needed and sync on who shall do it and when it shall be done.

sschleemilch commented 1 month ago

I merged it and added a point to the call tomorrow if we shall create a patch release and if so what artifacts that are needed and sync on who shall do it and when it shall be done.

Since colleagues are using vss-tools as a submodule, a release tag is probably not needed for them at all. Still it would probably good for transparency

erikbosch commented 1 month ago

Then we could use the lazy approach and just tag it, but (for now) not create a github release