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

Simplify name of generated exception mappers #44418

Merged
merged 1 commit into from
Nov 12, 2024
Merged

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Nov 11, 2024

Copy link

quarkus-bot bot commented Nov 11, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5a71def.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/panache/hibernate-reactive-rest-data-panache/deployment

io.quarkus.hibernate.reactive.rest.data.panache.deployment.repository.PanacheRepositoryResourcePutMethodTest.shouldUpdateComplexObject - History

  • 1 expectation failed. JSON path name doesn't match. Expected: is "updated collection" Actual: empty collection - java.lang.AssertionError
java.lang.AssertionError: 
1 expectation failed.
JSON path name doesn't match.
Expected: is "updated collection"
  Actual: empty collection

	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)

return targetMethod.declaringClass().name() + "$GeneratedExceptionHandlerFor$"
+ handledExceptionType.name().withoutPackagePrefix()
+ "$OfMethod$" + targetMethod.name();
return targetMethod.declaringClass().name() + "$ExceptionMapper$"
Copy link
Member

Choose a reason for hiding this comment

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

Why not replace $ExceptionMapper$ with the method name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find that looking at this name I know for sure that is a generated exception mapper without having to look into the code itself

Copy link
Member

Choose a reason for hiding this comment

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

Indeed you know what it is, but you don't know where it's defined, so you can't find it back easily.

Copy link
Member

Choose a reason for hiding this comment

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

Whereas if you can find it back, then it's pretty obvious what it is once you look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I say let's roll with the change as is and we can always come back if we find it confusing in practice

@geoand geoand merged commit 0037f52 into quarkusio:main Nov 12, 2024
46 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Nov 12, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Nov 12, 2024
@geoand geoand deleted the #43786 branch November 12, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rest: shorten the names of the generated classes
2 participants