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

Enhancements to Configuration section of the Logging guide #35988

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

MichalMaler
Copy link
Contributor

@MichalMaler MichalMaler commented Sep 18, 2023

Fixing the badly rendered asciidoc markup that should provide an escape on used asterixes. In my local build, this worked perfectly fine, but for some reasons, it is not rendere as expected on Quarkus portal (main snapshot)

Bad asciidoc translation
image

Expected asciidoc translation
image

To fix this, I wanted to use pass:[] asciidoc macro that would allow me to use * in the backtick and render properly (no bold in the text, not escape macro leftovers). The \ and ++ escape approaches weren't rendered properly, so I used the pass[] macro as my last resort.
I also wanted to reword the sentence slightly and put it in a bullet list:

An example of a global configuration that applies for all categories:

* `quarkus.log.console.pass:c,a[*]`
* `quarkus.log.file.pass:c,a[*]`
* `quarkus.log.syslog.pass:c,a[*]`

However, after consulting this with SME, we agreed on removing these handlers examples completely and replace then with the contextually correct examples (root and per-category) that better fits the overall context of this chapter.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 18, 2023

I don't have time to review new text. We need just the fix.

@MichalMaler MichalMaler added the area/docstyle issues related for manual docstyle review label Sep 18, 2023
@MichalMaler
Copy link
Contributor Author

Text is almost the same. I asked @jmartisk for a sanity check that I didn't change the meaning.

@Ladicek Ladicek removed their request for review September 18, 2023 11:30
@Ladicek
Copy link
Contributor

Ladicek commented Sep 18, 2023

Then I'm leaving it to him.

@github-actions
Copy link

github-actions bot commented Sep 18, 2023

🙈 The PR is closed and the preview is expired.

@MichalMaler
Copy link
Contributor Author

@jmartisk @cescoffier The GitHub Quarkus preview bot provides a nice and working asciidoc pass macro rendering.
Now I will apply the slight changes I talked with you over the googledoc.

gsmet
gsmet previously requested changes Sep 18, 2023
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.

I second what @Ladicek said.

Please fix the reported issue and avoid changing the content in a way that requires a closer review.

I have no idea what you mean by quarkus.log.console.pass:c,a[*] and really this fix should be straightforward and shouldn't need an extensive review and discussions.

@MichalMaler MichalMaler force-pushed the Logging-asciidoc-pass branch 2 times, most recently from a99edf5 to 423d4a8 Compare September 18, 2023 12:35
@MichalMaler MichalMaler changed the title Escaping the badly rendered Asciidoc makrup Escaping the badly rendered Asciidoc markup and adding better per-category configuration examples to the Logging guide. Sep 18, 2023
@MichalMaler
Copy link
Contributor Author

MichalMaler commented Sep 18, 2023

I second what @Ladicek said.

Please fix the reported issue and avoid changing the content in a way that requires a closer review.

I have no idea what you mean by quarkus.log.console.pass:c,a[*] and really this fix should be straightforward and shouldn't need an extensive review and discussions.

It is already reviewed by @cescoffier and @jmartisk . What I am using is the simple Asciidoc escape macro that I am using when \ or ++ escapes doesnt work. And yes, I already tried them. All worked in my local build, but it was not working as expected when displayed on Quarkus main-snashot docs portal.

My second commit I already squashed solve this by different structure, thus the escape macro is not needed (since there is nothing to escape now).

This small PR is fixing the problem with badly rendered asciidoc escape and provide more value trough better examples.

@gsmet Please, dont block the progress of this PR to be merged and backported.

@MichalMaler MichalMaler changed the title Escaping the badly rendered Asciidoc markup and adding better per-category configuration examples to the Logging guide. Working around the badly rendered Asciidoc markup and adding better per-category configuration examples to the Logging guide. Sep 18, 2023
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM but I don't know much about Asciidoc so I'm not gonna dismiss the blocking review

@MichalMaler
Copy link
Contributor Author

LGTM but I don't know much about Asciidoc so I'm not gonna dismiss the blocking review

The asciidoc passes are no longer needed, since I removed the whole text they were needed for.
If the content is OK, I dont see a reason for creating another PR in which I will be fixing asciidoc escapes for a content that is not good.

@geoand
Copy link
Contributor

geoand commented Sep 18, 2023

Yeah, I understand, but since this is not my field of expertise, I am not willing to dismiss concerns that others might have. If this were one of my things, I agreed with the change and I thought others concerns were either satisfied or not important enough to block the progress, I would dismiss the blocking review

Rewording the chapter about logging categories

Adding examples for the better context

Signed-off-by: Michal Maléř <[email protected]>
@MichalMaler
Copy link
Contributor Author

Creating this on the @gsmet request.
#35995

@gsmet
Copy link
Member

gsmet commented Sep 18, 2023

@MichalMaler well, next time, please don't ask me for a review on Zulip then. I really have better things to do.

@gsmet gsmet dismissed their stale review September 18, 2023 15:21

Let's stop wasting my time, please.

@MichalMaler MichalMaler changed the title Working around the badly rendered Asciidoc markup and adding better per-category configuration examples to the Logging guide. Enhancements to Configuration section of the Logging guide Sep 18, 2023
@MichalMaler
Copy link
Contributor Author

@MichalMaler well, next time, please don't ask me for a review on Zulip then. I really have better things to do.

I will choose to answer in a slightly different tone. I kindly asked for your opinion 5 hours ago before creating this PR. An application of asciidoc macro that is visible in the code was something I wanted to discuss with you briefly.
Then, I asked Jan and Ladislav to review the content I composed with @cescoffier .
I didn't request your review on this PR (look yourself in the history), because I didn't want to request more of your time on this, and I took the silence on your part as a signal of your business, which I understand completely.

As you wish, I will not ask you for a review again.

Have a great day, G.

@Sanne Sanne merged commit 8a0d070 into quarkusio:main Sep 19, 2023
5 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Sep 19, 2023
@MichalMaler MichalMaler deleted the Logging-asciidoc-pass branch September 20, 2023 09:45
@gsmet gsmet modified the milestones: 3.5 - main, 3.2.7.Final Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docstyle issues related for manual docstyle review area/documentation
Projects
Development

Successfully merging this pull request may close these issues.

9 participants