refactor: move output code into formatters, add junit and githubactions

[PhilippHeuer]

Creating this as a draft / suggestion for feedback to see if this approach works for you.

Changes Proposed

• move all code that deals with rendering output into formatters
• strip ansi escape codes in all data serialization formats (json, yaml, junit, ...) - i would only include these in text output
• add junit format (breaking-changes)
• add githubactions format (breaking-changes)

Incomplete

• have the checks command return a list of all checks - maybe add a filter param for optional checks?
• complete the list of checks

Additional Information

• a list of all rules would be useful (for junit/sarif) - including key, description - i added one as example in https://github.com/PhilippHeuer/oasdiff/blob/3a27bf871c6f9b3d07b86d3b3d56f2c0772e9899/checker/default_checks.go#L159-L169
• supporting the sarif format would require the file and location for the source - not sure if this will be possible (maybe if the files don't contain references), added the fields optionally for now in case this will become possible at some point in the future
• the method i used to strip the ansi escape codes is more of a hack than a solution

[jbergstroem]

It would be awesome if the githubactions reporter also added a count of error/warn/notice to variables (echo ERROR_COUNT=5 >> $GITHUB_ENV). That way the github action can make them available to github action users and you can use it in logic for further steps.

[PhilippHeuer]

It would be awesome if the githubactions reporter also added a count of error/warn/notice to variables (echo ERROR_COUNT=5 >> $GITHUB_ENV). That way the github action can make them available to github action users and you can use it in logic for further steps.

Good idea, i'll add a commit to set the values as job output parameters (instead of env) - this should be better to avoid potential name conflicts with other env variables.

[jbergstroem]

Good idea, i'll add a commit to set the values as job output parameters (instead of env) - this should be better to avoid potential name conflicts with other env variables.

Yes, output is superior here. Thanks for that!

[codecov-commenter]

Codecov Report

Merging #386 (5d1f433) into main (3618a31) will decrease coverage by 0.74%.
Report is 4 commits behind head on main.
The diff coverage is 69.65%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
- Coverage   81.59%   80.86%   -0.74%     
==========================================
  Files         201      208       +7     
  Lines       11592    12232     +640     
==========================================
+ Hits         9459     9891     +432     
- Misses       1784     1983     +199     
- Partials      349      358       +9     

Flag	Coverage Δ
unittests	80.86% <69.65%> (-0.74%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
checker/check-added-required-request-body.go	100.00% <100.00%> (ø)
checker/check-api-added.go	100.00% <100.00%> (ø)
checker/check-api-operation-id-updated.go	100.00% <100.00%> (ø)
checker/check-api-tag-updated.go	100.00% <100.00%> (ø)
checker/check-components-schemas-removed.go	100.00% <100.00%> (ø)
...er/check-new-request-non-path-default-parameter.go	100.00% <100.00%> (ø)
checker/check-new-request-non-path-parameter.go	100.00% <100.00%> (ø)
...cker/check-new-requried-request-header-property.go	91.89% <100.00%> (ø)
checker/check-request-body-became-enum.go	93.93% <100.00%> (ø)
checker/check-request-body-enum-deleted.go	88.88% <100.00%> (ø)
... and 99 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[reuvenharrison]

Hi @PhilippHeuer,
Please let me know if you need help to finish this PR.
FYI, I am about to merge another PR that may require some manual merging into this one.
Reuven

[PhilippHeuer]

I'll take a look today to push my latest changes + resolve the conflicts.

[PhilippHeuer]

Changes

• completes the list of all rules
• all checker functions now have a Check suffix
• all rule-id's are now constants and referenced in the rule list (/checks list)
• oasdiff checks now supports lang, severity, tags and required for filtering
  • oasdiff checks --severity error # list all checks with severity error
  • oasdiff checks --tags body # list all checks related to the body
  • oasdiff checks --required false # list all optional checks
• All rule's come with a <ruleId>-description localization key, that we can use to describe what the rule in general (not the specific error) - writing this is out-of-scope for this PR.

I think we shouldn't add any other changes to this PR if possible, as it's already quite big / hard to review.

Checkers

There are a lot minor changes in the Checkers to unify the rule-id's that were not very consistent as i needed to reference them in the checks list. (public const, camelCase, Id suffix)
Does this style work for you or would you prefer something else?

Idea for a Future PRs

1: use the new checks list in the junit report

2: oasdiff checks --required false includes two more items than the old oasdiff checks because one Checker function can return multiple rule-id's.
I was thinking that we could always run all Checkers and allow users to include optional / exclude required result items based on the rule-id's for more fine-grained control.

[reuvenharrison]

I'm reviewing...

[reuvenharrison]

Hi @PhilippHeuer,
Thanks for your valuable contribution.
The new formatters look great.
From my perspective it's ready to merge.
Reuven