-
Notifications
You must be signed in to change notification settings - Fork 140
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 XmlReport (JUnit) #190
base: main
Are you sure you want to change the base?
Conversation
Nice MR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work, @hesstobi -- I would urge you to use Python built-in XML instead of lxml
. For this project, it is better to keep dependencies at bare minimum. Also, main advantages of lxml would not make sense for us. As we are not parsing XML as well but just generating, we should be safe.
key=key) | ||
|
||
if output: | ||
with open(output, 'w+') as output_file: | ||
output_file.write(output_report) | ||
if xml: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this logic must be part of safety.formatter.report
. My understanding is that you did put this here because then we could have more than one output, which sounds great, but then we would need some extra refactoring.
@@ -22,7 +22,8 @@ | |||
'Click>=6.0', | |||
'requests', | |||
'packaging', | |||
'dparse>=0.4.1' | |||
'dparse>=0.4.1', | |||
'lxml' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike on how lxml depends on a binary library to work. 😕
This file available under the terms of the MIT License as follows: | ||
|
||
******************************************************************************* | ||
* Copyright (c) 2010 Thales Corporate Services SAS * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but it doesn't sounds necessary to merge this file with such license. I think we can abdicate the schema validation as part of our tests.
Hi @rafaelpivato , is there any chance to see this PR being finished and merged (without the author)? |
@harlekeyn can you look at this, please? |
Hi, any updates regarding the merge? |
Hi @0xjjoyy would you like fix/resolve in the code the comment reviews done by Rafael? I can review/test and merge the PR ASAP if those comments are resolved in the code. |
Add an XML output format with
--xml
. This creates a JUnit xml formated output. This makes it easy to display the test result for CI.