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

Qute Character Escapes for Json Content #43900

Closed
wants to merge 9 commits into from

Conversation

ogomezdi
Copy link
Contributor

Changes for covering JSON Escapes in templates mentioned in issue #43369

@quarkus-bot quarkus-bot bot added area/documentation area/qute The template engine labels Oct 16, 2024
Copy link

github-actions bot commented Oct 16, 2024

🙈 The PR is closed and the preview is expired.

@ogomezdi ogomezdi marked this pull request as ready for review November 21, 2024 17:53

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link

quarkus-bot bot commented Nov 25, 2024

Status for workflow Quarkus Documentation CI

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

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@gsmet
Copy link
Member

gsmet commented Nov 25, 2024

@mkouba not sure you are aware of this effort?

@mkouba
Copy link
Contributor

mkouba commented Nov 25, 2024

@mkouba not sure you are aware of this effort?

@gsmet I think I was but I forgot ;-).

@ogomezdi Could you squash the commits please?

@ogomezdi
Copy link
Contributor Author

@mkouba not sure you are aware of this effort?

@gsmet I think I was but I forgot ;-).

@ogomezdi Could you squash the commits please?
Sorry for the inconvenience, it was my first time and I make foolish typo errors in testing class.
The functionality was the one we spoke and agreed long ago @mkouba.
What exactly do you want me to do?

@mkouba
Copy link
Contributor

mkouba commented Nov 25, 2024

What exactly do you want me to do?

To squash the commits in one. See for example https://www.git-tower.com/learn/git/faq/git-squash or https://www.geeksforgeeks.org/git-squash/.

@ogomezdi
Copy link
Contributor Author

What exactly do you want me to do?

To squash the commits in one. See for example https://www.git-tower.com/learn/git/faq/git-squash or https://www.geeksforgeeks.org/git-squash/.

I'm not able to do this :( , my git skills are minimal, and executing commands in examples provided get me to nowhere.

Copy link

quarkus-bot bot commented Nov 25, 2024

Status for workflow Quarkus CI

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

✅ 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-vertx-exporter

io.quarkus.it.opentelemetry.vertx.exporter.grpc.SimpleGrpcNoTLSWithCompressionTest.test - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.opentelemetry.vertx.exporter.AbstractExporterTest Expecting Optional to contain a value but it was empty within 30 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.it.opentelemetry.vertx.exporter.AbstractExporterTest 
Expecting Optional to contain a value but it was empty within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)

@mkouba
Copy link
Contributor

mkouba commented Nov 26, 2024

What exactly do you want me to do?

To squash the commits in one. See for example https://www.git-tower.com/learn/git/faq/git-squash or https://www.geeksforgeeks.org/git-squash/.

I'm not able to do this :( , my git skills are minimal, and executing commands in examples provided get me to nowhere.

No worries, I can do that.

@mkouba mkouba mentioned this pull request Nov 26, 2024
@mkouba
Copy link
Contributor

mkouba commented Nov 26, 2024

This pull request is superseded by #44706.

@ogomezdi I wasn't able to merge the commits on your branch either. So I've created a new branch, cherry-picked your commits, rebased and introduced some changes:

  1. Added a test to EscapingTest to verify the Quarkus integration
  2. Introduced CharReplacementResultMapper that holds the common logic for JsonEscaper and HtmlEscaper.
  3. Modified the logic in JsonEscaper to improve the performance (quick benchmark showed the diff ~ 10%).
  4. Removed the duplicate parts from the docs.

Let me know what you think.

@mkouba mkouba closed this Nov 26, 2024
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Nov 26, 2024
@ogomezdi
Copy link
Contributor Author

This pull request is superseded by #44706.

@ogomezdi I wasn't able to merge the commits on your branch either. So I've created a new branch, cherry-picked your commits, rebased and introduced some changes:

  1. Added a test to EscapingTest to verify the Quarkus integration
  2. Introduced CharReplacementResultMapper that holds the common logic for JsonEscaper and HtmlEscaper.
  3. Modified the logic in JsonEscaper to improve the performance (quick benchmark showed the diff ~ 10%).
  4. Removed the duplicate parts from the docs.

Let me know what you think.

Great solution!!!
I didn't notice the need of "EscapingTest", simply perfect.
Creating an abstract class, makes easier to understand the logic, despite the performance improvement you've mentioned, and it's convenient for future escape needs in other target formats (YAML, ...)

Thanks a lot!!!
I promise I'll improve my git skills for future pull request.

@mkouba
Copy link
Contributor

mkouba commented Nov 26, 2024

This pull request is superseded by #44706.
@ogomezdi I wasn't able to merge the commits on your branch either. So I've created a new branch, cherry-picked your commits, rebased and introduced some changes:

  1. Added a test to EscapingTest to verify the Quarkus integration
  2. Introduced CharReplacementResultMapper that holds the common logic for JsonEscaper and HtmlEscaper.
  3. Modified the logic in JsonEscaper to improve the performance (quick benchmark showed the diff ~ 10%).
  4. Removed the duplicate parts from the docs.

Let me know what you think.

Great solution!!! I didn't notice the need of "EscapingTest", simply perfect. Creating an abstract class, makes easier to understand the logic, despite the performance improvement you've mentioned, and it's convenient for future escape needs in other target formats (YAML, ...)

Thanks a lot!!!

You're welcome and thanks for your contribution!

I promise I'll improve my git skills for future pull request.

💯 ;-)

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.

3 participants