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

Handle text blocks when creating Panache find query #31419

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

bakrhaso
Copy link
Contributor

@bakrhaso bakrhaso commented Feb 25, 2023

I filed this issue #31210 from my work account. This PR should fix it. I'm not really sure why one test is failing. As far as I can tell it doesn't seem related to my change, but this is my first time contributing so hopefully someone else can shed some light on that.

Fixes #31210

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 25, 2023

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@bakrhaso bakrhaso changed the title replace \r and \n with spaces so creating query works with text blocks Fix #31210: Handle text blocks when creating Panache find query Feb 25, 2023
@bakrhaso bakrhaso marked this pull request as ready for review February 25, 2023 21:25
@gsmet gsmet requested review from FroMage and loicmathieu February 27, 2023 12:42
@gsmet gsmet changed the title Fix #31210: Handle text blocks when creating Panache find query Handle text blocks when creating Panache find query Feb 27, 2023
Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Could we test this in the Java tests because they are our reference?

Thanks.

@bakrhaso
Copy link
Contributor Author

bakrhaso commented Mar 3, 2023

I'm not sure if the formatter supports text blocks in Java, it seems to fail no matter what I do.

Shall I remove the Kotlin tests? They are pretty much identical.

@loicmathieu
Copy link
Contributor

Quarkus supports Java 11.
For tests for features on more recent versions there is a java17 integration test that can be used.

@bakrhaso
Copy link
Contributor Author

I've added the Java tests

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 13, 2023

Failing Jobs - Building d0d60cb

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Build Failures Logs Raw logs
✔️ Gradle Tests - JDK 11 Windows
Native Tests - Data1 Build ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.MultiModuleWithEmptyModuleDevModeTest.main line 22 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

@loicmathieu
Copy link
Contributor

@gsmet the CI failure seems to be non-related. Is it OK for you to merge it ?

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Yes, let's merge it!

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using PanacheQuery#project with multi-line string in Kotlin fails
3 participants