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

added a json flag #58

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

added a json flag #58

wants to merge 7 commits into from

Conversation

vdbaan
Copy link

@vdbaan vdbaan commented May 4, 2022

Added this json flag so that the output of pipal can be used in a pipeline.

@digininja
Copy link
Owner

I like the idea, but not the way it is implemented. Copy/paste on the whole get_results functions gives a lot of redundant code and a high chance of things getting out of sync if a change is made to one of the functions but not the other.

There is also data missing from the JSON, for example around line 390 on the basic checker, you've commented out the string output but not replaced it with JSON output.

I'm happy to add JSON output, I just think it needs doing in a more generic way. Maybe change the main get_results to return JSON and then parse that into text if that is what is required. That way, if someone a different style of output they could add a new JSON parser to create whatever they wanted.

Sorry to be negative, but I just don't like the idea of supporting so much repeated code, even though there hasn't really been any changes to the checkers for a long time.

@vdbaan
Copy link
Author

vdbaan commented May 4, 2022

I don't see it as negative comment, but constructive.

I would recommend not changing the output of get_results to json as that could mess up the other checkers, but to made it call get_json_output and then show the output from that as it would normally.

And thanks for the catch on line 390, will amend that.

@digininja
Copy link
Owner

I would recommend not changing the output of get_results to json as that could mess up the other checkers, but to made it call get_json_output and then show the output from that as it would normally.

That would work as well.

@vdbaan
Copy link
Author

vdbaan commented May 4, 2022

Reworked get_results to print based on the get_json_results.

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