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

Add file location from XML elements to the issues in checks #124

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

johannesWen
Copy link

@johannesWen johannesWen commented Dec 18, 2024

Description

Added also the file location of the XML elements to issues, so that result file contains this information without calling the result pooling before.

Belongs to discussion in qc-baselib-py PR: asam-ev/qc-baselib-py#51

How was the PR tested?

  1. pytests successful

Signed-off-by: Johannes Wenninger <johannes.wenninger@lcm.at>
Signed-off-by: Johannes Wenninger <johannes.wenninger@lcm.at>
@andreaskern74
Copy link
Collaborator

@MatteoRagni / @pmai / @hoangtungdinh : What do you think about this contribution? See also discussion in asam-ev/qc-baselib-py#51 . This could be somehow wrapped in an own function, because it's always the same. But it general I like to add the filelocation directly in the checker library, because in Python the information is directly available. No need to wait for the call to the Result Pooling if only one checker library is used.

@andreaskern74 andreaskern74 added the isState:ForCCBReview CCB will review it and change the status to ReadyForMerge if everything is ok label Dec 18, 2024
Copy link
Contributor

@hoangtungdinh hoangtungdinh left a comment

Choose a reason for hiding this comment

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

Hi @johannesWen, @andreaskern74,

Thank you for your contribution, @johannesWen. I think the change looks great!

Ideally, we can change qc-baselib-py to add a function add_locations, which adds both xpath and file location based on lxml elements. It is also related to this issue.

But I think this PR is good to be merged already.

@andreaskern74
Copy link
Collaborator

andreaskern74 commented Dec 18, 2024

Adapted the description and the subject to the implementation.

@johannesWen : We need three approvals from CCB members to merge the PR. Maybe it takes a little bit longer due the holidays. Let's see....

@andreaskern74 andreaskern74 changed the title added lines argument to add_xml_location in checks Add file location from XML elements to the issues in checks Dec 18, 2024
@johannesWen
Copy link
Author

@andreaskern74
Sounds great and I wish you a nice vacation 🎄

@pmai
Copy link
Collaborator

pmai commented Dec 19, 2024

Hi @johannesWen, @andreaskern74,

Thank you for your contribution, @johannesWen. I think the change looks great!

Ideally, we can change qc-baselib-py to add a function add_locations, which adds both xpath and file location based on lxml elements. It is also related to this issue.

But I think this PR is good to be merged already.

I would agree that this is something that should ideally be handled in baseline going forward, either as suggested with a common helper function, or by having a sub-class relationship that makes xml locations optionally also have file location info.

But as @hoangtungdinh says, in the interim it makes sense to merge this PR from my side.

Copy link
Collaborator

@pmai pmai left a comment

Choose a reason for hiding this comment

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

Looks good for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
isState:ForCCBReview CCB will review it and change the status to ReadyForMerge if everything is ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants