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

Eliminating the white space from the xref macro #35084

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

MichalMaler
Copy link
Contributor

@MichalMaler MichalMaler commented Jul 28, 2023

The correct way of using xref macro is documented in https://quarkus.io/guides/doc-reference#anchors-reference
This PR just cleans it up so that contributors can not get a bad example directly from the source code.

Kudos to @jmartisk for creation of the regex to catch them all.

@MichalMaler MichalMaler requested review from gsmet and gastaldi July 28, 2023 11:55
@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation labels Jul 28, 2023
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Good catch!

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

Question: Should we have a linter with such kind of rules?

@michelle-purcell
Copy link
Contributor

Question: Should we have a linter with such kind of rules?

Great idea @cescoffier . We could investigate adding this to the Vale ruleset for Quarkus.
@MichalMaler - WDYT? let's try this out... 🧪
BTW... a PR is on the way to fix most of the false-positives too.

@gsmet
Copy link
Member

gsmet commented Jul 28, 2023

Does it hurt to have a whitespace? Just wondering how much we should invest in it.

@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 28, 2023
@MichalMaler
Copy link
Contributor Author

Does it hurt to have a whitespace? Just wondering how much we should invest in it.

I guess it doesn't hurt, but I like to keep things unified if possible.
The recommendation to not to use the space after comma come directly from you; you told me this while i was drafting the cross-ref guide (March 22)

@gsmet
Copy link
Member

gsmet commented Jul 28, 2023

Well, I don't recall saying that tbh. I said it was working with or without equally but the doc is without and that our example should be consistent with the AsciiDoctor doc.

I'm not saying we shouldn't merge this, it's good. I wouldn't make it a priority to have a tooling to check that though, if it doesn't cause any problems, given all the other issues we have that actually cause problems.

@MichalMaler
Copy link
Contributor Author

MichalMaler commented Jul 28, 2023

Well, I don't recall saying that tbh. I said it was working with or without equally but the doc is without and that our example should be consistent with the AsciiDoctor doc.

I'm not saying we shouldn't merge this, it's good. I wouldn't make it a priority to have a tooling to check that though, if it doesn't cause any problems, given all the other issues we have that actually cause problems.

image

But you are saying in next sentence that you dont think it matters.
So, as I said, this is just a cosmetic tweak, clean-up, unification, to reach some consistency. :)

Vale can use this as a recommendation, or I can do this check once a year, or drop this completely.
No hard feelings if this will not got merged.

@gsmet
Copy link
Member

gsmet commented Jul 28, 2023

Looks like I had a good recollection of what I said :)

I'm not sure it's coming from these changes but the doc build is failing with:

[INFO] asciidoctor: INFO: possible invalid reference: duplicated-context
[INFO] asciidoctor: INFO: possible invalid reference: duplicated-context

@gsmet
Copy link
Member

gsmet commented Jul 28, 2023

We need to understand what's going on with ^ but we can merge once it's fixed.

@MichalMaler
Copy link
Contributor Author

MichalMaler commented Jul 28, 2023

Looks like I had a good recollection of what I said :)

You are my greatest Quarkus hero, would never said something you didn't :))

All I did was use this regex to search for the space after a comma inside <<>> in the repo .adoc files with:
(<<[A-Za-z-]+,)
And replace it with:
$1

The truth is, this operation had two iterations (but it was maybe caused by the fact that my IDEA didn't find all occurrences when I hit the FindNReplace button for the first time.

I double-checked all stuff I touched and they are all in the <<>>, thus should not touched the context or and ID.
My local build with mvn clean package finished OK too.

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.

Merging as the failure is not related. I will fix it in an upcoming PR.

@gsmet gsmet merged commit 37b87aa into quarkusio:main Jul 28, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 28, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 28, 2023
@github-actions
Copy link

🙈 The PR is closed and the preview is expired.

@gsmet
Copy link
Member

gsmet commented Jul 28, 2023

See #35089

@MichalMaler MichalMaler deleted the CommaXrefFix branch July 28, 2023 15:29
@michalvavrik
Copy link
Member

This PR just cleans it up so that contributors can not get a bad example directly from the source code.

Yeah, that's exactly how I work :-) Thanks!

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
None yet
Development

Successfully merging this pull request may close these issues.

8 participants