search

asam-ev / qc-framework

Mozilla Public License 2.0
13 stars 6 forks source link

Add report module GitHub ci #95

Open ClemensLinnhoff opened 1 week ago

ClemensLinnhoff commented 1 week ago

Description

Add report module to output issues from a report in the GitHub pipeline syntax.

How was the PR tested? I set up a test branch with a CI pipeline giving examples/viewer_example/resources/Result.xqar as an input. You can see the output of the pipeline with the different issue levels (error, warning, info) here.

image

Fixes #92

ClemensLinnhoff commented 1 week ago

Check this out @jdsika.

jdsika commented 1 week ago

Cool 😎

ClemensLinnhoff commented 1 week ago

I think it is very interesting to have this kind of implementation, which was already partially discussed in the Framework Meetings.

However, I would put this in draft. The framework has not yet reached a stable api (we have not even reached a "complete" api), and some of the objects used in this report module may abruptly change.

Merging the report module will make it part of the project (and it should be part of the project), and we will have the responsability to maintain it. I would not widen the maintenance base at this early stage of the project. I would merge it in a later stage, when things will become more stable.

I'm adding the CCB label, to discuss it together.

Thank you for your review @MatteoRagni! I implemented all suggestions.

Regarding the merge: I was under the impression that this project will be finalized the next month. So I assumed, that the output format was already pretty much final. But if it is not, I understand that you would like to keep this implementation on a separate branch until it is. Any changes in the output format should be straightforward to implement in this module then.

MatteoRagni commented 1 week ago

Regarding the merge: I was under the impression that this project will be finalized the next month. So I assumed, that the output format was already pretty much final. But if it is not, I understand that you would like to keep this implementation on a separate branch until it is. Any changes in the output format should be straightforward to implement in this module then.

You are correct, an initial release will be finalized. I'm confident that after we can discuss the merge of your contribution. But I cannot guarantee compatibility before, and the service provider team is really pushing to implement the checker rules (for OpenDRIVE and OpenSCENARIO) considered as targets for this initial release.

ClemensLinnhoff commented 1 week ago

Okay I am just going to wait a while for further comment until I implement them. Every day new comments come in. I'd rather implement every review comment at once than 3 every day...

MatteoRagni commented 5 days ago

Hey @ClemensLinnhoff, we just discussed the PR in workgroup.

The code implemented uses an API that should be considered legacy, and we have in roadmap to update it. It will require some time, thuis this contribution can be considered a test bench to see what will break during changes. The contribution will be used also as an updated "example" on how to implement report modules, superseeding the text report module current implementation.

If you complete the review, and everything seems correct, I'll put it in CCB review stack, to get the final approve for merge.

ClemensLinnhoff commented 5 days ago

Hey @ClemensLinnhoff, we just discussed the PR in workgroup.

The code implemented uses an API that should be considered legacy, and we have in roadmap to update it. It will require some time, thuis this contribution can be considered a test bench to see what will break during changes. The contribution will be used also as an updated "example" on how to implement report modules, superseeding the text report module current implementation.

If you complete the review, and everything seems correct, I'll put it in CCB review stack, to get the final approve for merge.

Done