Make io.quarkus.builder.Consume a record

[punkratz312]

This pull request reduces boilerplate code and eliminates unused code, addressing concerns related to code clarity and maintenance.
This change aims to streamline the codebase while maintaining functionality.

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Quarkus - Core 999-SNAPSHOT:
[INFO] 
[INFO] Quarkus - Development mode - SPI ................... SUCCESS [  8.445 s]
[INFO] Quarkus - Core - Class Change Agent ................ SUCCESS [  0.405 s]
[INFO] Quarkus - IDE Launcher ............................. SUCCESS [ 25.755 s]
[INFO] Quarkus - Builder .................................. SUCCESS [  3.064 s]
[INFO] Quarkus - Core ..................................... SUCCESS [  0.098 s]
[INFO] Quarkus - Core - Extension Processor ............... SUCCESS [ 15.461 s]
[INFO] Quarkus - Core - Runtime ........................... SUCCESS [  7.584 s]
[INFO] Quarkus - Core - Deployment ........................ SUCCESS [ 34.227 s]
[INFO] Quarkus - JUnit 4 Mock ............................. SUCCESS [  0.241 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:37 min
[INFO] Finished at: 2025-01-16T08:53:03+01:00
[INFO] ------------------------------------------------------------------------

[punkratz312]

@gastaldi please lets finally get this done, thanks.

[gastaldi]

@punkratz312 I am not sure how you are building the project, but wasn't formatted properly.

Anyway, I built with mvn -Dquickly and force pushed to your branch, the build should pass now

[gastaldi]

Ooops, forgot to commit the changes 😅

[gsmet]

Frankly, I'm not sure it's a good idea to merge this: the value is very narrow and I'm not excited about all the conflicts we will end up having if we start converting a lot of classes to records.

Why this class? Does it actually bring some serious improvements?

Now @gastaldi I will let you judge of this particular case but let me be clear: I don't want us to convert a lot of classes to records, we can adjust when there's real value but we have to take into account the potential maintenance burden of handling the backports and potential conflicts with the getter name changes. If it makes things awesomely better then we can swallow this burden, if it doesn't, why would we?

[gsmet]

@punkratz312 if I had an advice for you, it would be: if you actually want to contribute to Quarkus, try to handle a bug open in our tracker and fix it. That would be more valuable for us than changing a random class in the tree to a record.

I also think it would be a better learning experience for you.

[gastaldi]

@gsmet I agree it doesn't add much value. However, accepting this PR will not hurt, given this class is package-private.

@punkratz312 if I had an advice for you, it would be: if you actually want to contribute to Quarkus, try to handle a bug open in our tracker and fix it. That would be more valuable for us than changing a random class in the tree to a record.

I also think it would be a better learning experience for you.

@punkratz312 if you're looking for an issue to work on, take a look at the ones labeled good first issue: https://github.com/quarkusio/quarkus/issues?q=sort%3Aupdated-desc%20is%3Aissue%20is%3Aopen%20label%3A%22good%20first%20issue%22

[punkratz312]

I see valid arguments in every direction. However, my main point is to remove the dead code, which was the original purpose of initiating this PR. Subsequently, the suggestion to adopt the record approach refined it into the most minimalistic solution possible. You can't take anything away from it without breaking everything. This adheres to the principle of having only what's necessary and avoiding overhead, which aligns with Quarkus' philosophy—minimizing imports and focusing on performance. It's fascinating what it's already achieving in terms of optimization.

Of course, this change could raise the question: why not use the record approach universally if it's so beneficial? In my humble opinion, it comes down to maintainability. IDE warnings always catch my attention, and refactoring (the second phase of TDD) helps eliminate redundancy and replication, which is something I strongly advocate for. I don’t see how this internal change would cause any conflicts since it's encapsulated within the .jar.

---

PS:
OpenRewrite could address this very easily. Reducing boilerplate code helps maintain a leaner codebase, and the compiler benefits as it no longer has to compile unused source code into bytecode or optimize it unnecessarily.

Of course, this is not about introducing this framework—just wanted to mention it to give a broader picture.

https://wiki.c2.com/?TestDrivenDevelopment

[gastaldi]

@punkratz312 that's not dead code, the code is being used in other places, as you have noticed since you changed 3 files.

Migrating everything to record is a bad idea when you have API changes, since it will break existing code depending on it (this PR is an exception since the class is used internally only).

We already use OpenRewrite in the quarkus update command (see the recipes in https://github.com/quarkusio/quarkus-updates)

[punkratz312]

not all the coding, but some getters are marked as unused.

[punkratz312]

A lot of unused and unnecessary boilerplate makes it hard to see that it's actually just one tiny function.
Here, I have to scroll and feel overwhelmed while ignoring 90% of the code.

With records, it's a blink of an eye to understand, which is what maintenance should be about.
We write the code once but read it many times, so this should be the priority in my humble opinion.

The focus should be to make the code easy to maintain and understand for humans.
The computer doesn’t care about readable or logical code—it’s just doing its thing without asking questions.

[geoand]

This needs a rebase as there is a conflict

[punkratz312]

resolved

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ 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

📦 integration-tests/opentelemetry

✖ io.quarkus.it.opentelemetry.MetricsTest.directCounterTest - History

• Condition with Lambda expression in io.quarkus.it.opentelemetry.MetricsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.MetricsTest was not fulfilled within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.MetricsTest.directCounterTest(MetricsTest.java:57)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

---

⚙️ JVM Tests - JDK 21

📦 integration-tests/opentelemetry

✖ io.quarkus.it.opentelemetry.MetricsTest.directCounterTest - History

• Condition with Lambda expression in io.quarkus.it.opentelemetry.MetricsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.MetricsTest was not fulfilled within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.MetricsTest.directCounterTest(MetricsTest.java:57)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

---

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/grpc-hibernate

✖ com.example.grpc.hibernate.BlockingRawTest.shouldAdd - History

• Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at com.example.grpc.hibernate.BlockingRawTestBase.shouldAdd(BlockingRawTestBase.java:59)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

[punkratz312]

All checks have passed
1 neutral, 1 skipped, 50 successful checks

looks good now, right?

[punkratz312]

thanks