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

Fix primitive class handling in Serialisers #37788

Merged
merged 1 commit into from
Dec 22, 2023
Merged

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Dec 16, 2023

Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

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

I prefer this one as well!!! Thanks @geoand

@geoand
Copy link
Contributor Author

geoand commented Dec 16, 2023

🙏

@geoand geoand requested a review from michalvavrik December 16, 2023 17:27
@@ -18,7 +18,7 @@ public class ResponseInfoResource {

@Path("/simple")
@GET
public String get(@QueryParam("abs") String abs) {
public boolean get(@QueryParam("abs") String abs) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a test instead of changing an existing one?

Or this is safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's totally safe because the response itself was not being used anywhere and now it is.

I also checked that with this test the previous version of the code was failing

@franz1981
Copy link
Contributor

@geoand please check if there was some useless private empty method which was used in the Serialisers https://github.com/quarkusio/quarkus/pull/37778/files#diff-4331c4565452796771e5f021cc452593f1cbb4ee69557f63868aae37641769c6L263

@geoand
Copy link
Contributor Author

geoand commented Dec 16, 2023

There don't seem to be any

@franz1981
Copy link
Contributor

franz1981 commented Dec 16, 2023

I am surprised...the diff I have posted was with my change that was removing that dangerous used empty method (and replacing the usages with the right one)...where is gone?

... minutes later ...

Found it!

So it should be declared there and replaced with the one you have added

If we had no failures despite it we are lacking some tests

This comment has been minimized.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

I've checked and this fixes my issue.

@geoand
Copy link
Contributor Author

geoand commented Dec 18, 2023

@franz1981 thanks! Fixed

Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

quarkus-bot bot commented Dec 18, 2023

Failing Jobs - Building 004d51d

Status Name Step Failures Logs Raw logs Build scan
Native Tests - gRPC Update Docker Client User Agent ⚠️ Check → Logs Raw logs 🚧

@geoand geoand requested a review from gsmet December 18, 2023 07:33
@michalvavrik
Copy link
Member

Would it be possible to get it in, please?

@michalvavrik
Copy link
Member

geoand requested a review from gsmet 10 hours ago

ah, I didn't mention of this, okay let's wait, just annoying to have CI failures

@geoand
Copy link
Contributor Author

geoand commented Dec 19, 2023

Yeah, let's wait for a review from @gsmet

@michalvavrik
Copy link
Member

Yeah, let's wait for a review from @gsmet

Sure, np, I'll disable tests in out TS and we can enable them when @gsmet find a time to review. Thanks for the fix.

@gsmet
Copy link
Member

gsmet commented Dec 22, 2023

Sorry for the delay!

@gsmet gsmet merged commit b99d1a0 into quarkusio:main Dec 22, 2023
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 22, 2023
@geoand
Copy link
Contributor Author

geoand commented Dec 22, 2023

🙏

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.

4 participants