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

Fix ListMetadata #112

Merged
merged 3 commits into from
Jan 2, 2025
Merged

Conversation

erikbosch
Copy link
Contributor

@erikbosch erikbosch commented Nov 29, 2024

This intends to make the implementation consistent with the documentation.

In short, Databroker does not know if A.B refer to a signal A.B or a branch A.B. We solve that by both looking for A.B.** and A.B.
That is OK as if there is a signal A.B there cannot be a branch A.B, even if it may require somewhat more comparisons, but you cannot do it in one comparision using glob.

Also our current glob cargo package does not seem to be maintained, we may consider migrating to something else.

How to test:

Run the three commands below on latest released version as well as with this PR


# This one previously failed as we did not add .**

docker run --network=host fullstorydev/grpcurl  -d '{ "root": "Vehicle.Cabin.Sunroof" }' -plaintext localhost:55555 kuksa.val.v2.VAL.ListMetadata

# This one worked before as we added .** if path did not contain dots

docker run --network=host fullstorydev/grpcurl  -d '{ "root": "Vehicle" }' -plaintext localhost:55555 kuksa.val.v2.VAL.ListMetadata

# This one worked before as it reference a signal

docker run --network=host fullstorydev/grpcurl  -d '{ "root": "Vehicle.Speed" }' -plaintext localhost:55555 kuksa.val.v2.VAL.ListMetadata

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 94.69697% with 7 lines in your changes missing coverage. Please review.

Project coverage is 60.06%. Comparing base (7911268) to head (e0031c0).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
databroker/src/glob.rs 89.39% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   59.49%   60.06%   +0.57%     
==========================================
  Files          33       33              
  Lines       16071    16168      +97     
==========================================
+ Hits         9561     9712     +151     
+ Misses       6510     6456      -54     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@erikbosch erikbosch force-pushed the erik_lmd branch 2 times, most recently from 9617eae to f633f47 Compare November 29, 2024 16:18
@erikbosch erikbosch marked this pull request as ready for review November 29, 2024 16:23
@erikbosch erikbosch force-pushed the erik_lmd branch 2 times, most recently from 14a8e6c to 29d6471 Compare December 5, 2024 09:20
This intends to make the implementation consistent with the documentation.

In short, Databroker does not know if A.B refer to a signal A.B or a branch A.B.
We solve that by both looking for A.B.** and A.B.
That is OK as if there is a signal A.B there cannot be a branch A.B,
even if it may require somewhat more comparisons, but you cannot do it in one comparision using glob.

Also our current glob cargo package does not seem to be maintained,
we may consider migrating to something else.
Otherwise quite useless if you request data with wildcard
Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

lgtm 🍏

@SebastianSchildt SebastianSchildt merged commit a67f4e6 into eclipse-kuksa:main Jan 2, 2025
23 checks passed
@erikbosch erikbosch deleted the erik_lmd branch January 3, 2025 09:06
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