Skip to content

Conversation

@raffaelecarelle
Copy link
Contributor

Added support for violation reporting in both text and JSON formats by introducing a Printer interface and implementing TextPrinter and JsonPrinter classes. Refactored relevant methods to utilize the new Printer system and updated CLI options to allow format selection. Included comprehensive unit tests for the new functionality.

Added support for violation reporting in both text and JSON formats by introducing a `Printer` interface and implementing `TextPrinter` and `JsonPrinter` classes. Refactored relevant methods to utilize the new `Printer` system and updated CLI options to allow format selection. Included comprehensive unit tests for the new functionality.
Updated variable names in JsonPrinter to improve readability and better reflect their purpose. This change enhances the maintainability of the code without modifying functionality.
@micheleorselli
Copy link
Member

It looks good to me, just out of curiosity: is there a particular use case for the json output?

@raffaelecarelle
Copy link
Contributor Author

Hi @micheleorselli, for me the scope of the feature is to use with gitlab ci and code quality format supported is json.

https://docs.gitlab.com/ci/testing/code_quality/#import-code-quality-results-from-a-cicd-job

thank you

@micheleorselli
Copy link
Member

Hey @raffaelecarelle, before merging would you mind:

  1. moving the Printer directory under the CLI one?
  2. add an e2e test here to check the new option --format=json ?

Thanks 🙇

Introduces a new `--only-errors` CLI option to suppress non-essential output and display only errors. Refactors `Printer` classes into a dedicated `CLI\Printer` namespace to improve structure and organization.
@raffaelecarelle
Copy link
Contributor Author

Hey @raffaelecarelle, before merging would you mind:

1. moving the `Printer` directory under the `CLI` one?

2. add an e2e test [here](https://github.com/phparkitect/arkitect/blob/main/tests/E2E/Cli/CheckCommandTest.php) to check the new option `--format=json `?

Thanks 🙇

Hi, I realized that in order to have a valid json, it is necessary to suppress all irrelevant output. So I added another optione --only-errors to do this!

As you suggested then, I added an e2e and moved the printer under the CLI folder.

Let me know.

@micheleorselli
Copy link
Member

my 2 cent: it feels a bit strange having to specify two options in order to get the right output. it would be nice that if I specify --format=json then I only get json as output, no additional option needed
@AlessandroMinoccheri @fain182 wdyt?

@raffaelecarelle
Copy link
Contributor Author

I too initially thought this but preferred to add an option for clarity and greater flexibility. Anyway you tell me, no problem with unifying.

@fain182
Copy link
Collaborator

fain182 commented Mar 26, 2025

I think that to make the --format=json option easier to use, we should ensure it automatically includes the behavior of --only-errors. And if we can't think of any use cases for using --only-errors on its own, I’d suggest removing it.

So I agree with you @micheleorselli

The "only-errors" option was redundant and has been replaced by logic tied to the output format being JSON. This simplifies the code by reducing unnecessary options and aligns behavior directly with the selected format.
@raffaelecarelle
Copy link
Contributor Author

raffaelecarelle commented Mar 27, 2025

ok guys, I remove the --only-errors option.

As you said, this make easier option json to use.

ps if other format will added (ex XML), is enought to add it in this condition

 $onlyErrors = Printer::FORMAT_JSON === $format;

@micheleorselli micheleorselli merged commit 2e5f684 into phparkitect:main Mar 28, 2025
16 checks passed
@micheleorselli
Copy link
Member

thanks @raffaelecarelle!

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.

3 participants