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

Make revapi report functional #26678

Closed
wants to merge 10 commits into from

Conversation

zedbeit
Copy link
Contributor

@zedbeit zedbeit commented Jul 12, 2022

The idea is to improve revapi reporting.

Some screenshots of how the report looks like now.

img1

img3
img4
img5

Copy link

@metlos metlos left a comment

Choose a reason for hiding this comment

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

This looks ok... Could also please provide some rendered examples in the PR description so that we have a better idea about the reports?

@@ -3,7 +3,7 @@
<analysisConfiguration>
<revapi.java>
<missing-classes>
<behavior>error</behavior>
Copy link
Member

Choose a reason for hiding this comment

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

specific reason why changing this?

@maxandersen
Copy link
Member

i'm fine merging this - its miles better than current one :)

just that one question on chaning from error to report I wanted to understand.

@zedbeit
Copy link
Contributor Author

zedbeit commented Aug 18, 2022

The reason is for revapi to report missing classes not to fail the build

@metlos
Copy link

metlos commented Aug 22, 2022

Note that report is the default behavior. I'm not sure why error was put in place in Quarkus config, but the effect of error is essentially an exception during the analysis, which IMHO is not that useful. report will report the missing classes as part of the analysis results so that they can be worked with in the reports.

@metlos
Copy link

metlos commented Aug 22, 2022

One thing that I would probably try to add is the "example use-chain" that Revapi produces for the missing classes (and other differences, see reportUsesFor in https://revapi.org/revapi-java/0.27.0/configuration.html). This may be helpful for figuring out why a class is considered a part of the API by Revapi.

@zedbeit
Copy link
Contributor Author

zedbeit commented Aug 22, 2022

Thank you @metlos for your inputs.

@maxandersen
Copy link
Member

reason for fail/error was whne running pr builds the build would not pass if missing documentation for api changes.

I thought this was the way to do so ?

@maxandersen maxandersen marked this pull request as ready for review August 23, 2022 13:40
@maxandersen
Copy link
Member

@zedbeit can you rebase this then i'll merge it and we can look at example-use-chain in separate pr?

@zedbeit
Copy link
Contributor Author

zedbeit commented Aug 23, 2022

Ok done.

@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Sep 5, 2022
@zedbeit
Copy link
Contributor Author

zedbeit commented Sep 5, 2022

Added list of dependencies added/removed to the diff list.

This is how it now looks:
newchanges
addedDep

removed

@quarkus-bot

This comment has been minimized.

@zedbeit
Copy link
Contributor Author

zedbeit commented Oct 4, 2022

Added changed/updated dependencies to the diff list.

This is how it now looks:
depChanged
changedDependencies

@maxandersen
Copy link
Member

sorry did not notice the extra commits before now. Makes a big diff on the change log. awesome.

@maxandersen
Copy link
Member

@zedbeit can you squash the commits then we can merge it?

@zedbeit
Copy link
Contributor Author

zedbeit commented Oct 20, 2022

Made a new PR for this here, closing this one @maxandersen

@zedbeit zedbeit closed this Oct 20, 2022
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants