Repository navigation
Conversation
b2fe45b to
6ab5f93
Compare
The example table listed expected outputs that the implementation has never
produced for patterns where the number preceding '*' is not 1:
%c{1.2.*} documented o.a.c.test.Foo actual o.ap.commons.test.Foo
%c{1.3.*} documented o.a.commons.test.Foo actual o.apa.commons.test.Foo
%c{1.8.*} documented org.apache...Foo actual o.apache...Foo
'%c{1.1.1.*}' only appeared correct because all three counts happened to be 1.
NameAbbreviator treats each numeric token as the maximum number of characters to
print from the corresponding component (a component that is not longer than its
number is printed in full), and '*' means that component and every remaining one
is printed in full. The rightmost component is subject to the last token as well.
Also reword the prose above the table: it described the number before '*' as the
count of rightmost tokens printed in full, which does not match the implementation
and is the likely source of the confusion.
Verified against log4j-core 2.26.1 by invoking NameAbbreviator directly for every
row of the table; all 13 rows agree with the implementation.
No behaviour change: the existing NameAbbreviatorTest assertions are untouched.
Refs apache#2522
6ab5f93 to
d096358
Compare
|
Friendly ping for review when you have a moment. This is a docs-only fix (1 file, +7/−4) that aligns the |
|
The PR is right that |
The three examples that spell a period before the asterisk select
PatternAbbreviator, whose output does not match the table: against
log4j-core 2.26.1 %c{1.2.*} prints o.ap.commons.test.Foo, not
o.a.c.test.Foo, and likewise for 1.3.* and 1.8.*.
The prose above the table describes DynamicWordAbbreviator semantics
("how many of the rightmost tokens will be printed in full"), which is
the abbreviator selected when the asterisk directly follows the digit.
Writing the patterns as %c{1.2*}, %c{1.3*} and %c{1.8*} therefore makes
the prose and the documented output agree, without rewriting either.
Verified against log4j-core 2.26.1 via NameAbbreviator.getAbbreviator
(logger org.apache.commons.test.Foo):
%c{1.2*} -> o.a.c.test.Foo (documented: o.a.c.test.Foo)
%c{1.3*} -> o.a.commons.test.Foo (documented: o.a.commons.test.Foo)
%c{1.8*} -> org.apache.commons.test.Foo (documented: org.apache.commons.test.Foo)
%c{1.2.*} -> o.ap.commons.test.Foo (does NOT match documented value)
Documentation only; no behaviour change.
Refs apache#2522
b601dc3 to
22a1366
Compare
|
Thanks @ramanathan1504 — you're right, and I've gone with your suggestion. I pushed a new head that reverts my prose rewrite entirely and only changes the three patterns to Your point about the two abbreviators was the piece I had wrong: the period routes to I left PR description updated to match. Thanks for the careful read. |
What
The three
%cprecision examples inpattern-layout.adocthat spell a period before the asterisk now use the form without it:%c{1.2.*}%c{1.2*}%c{1.3.*}%c{1.3*}%c{1.8.*}%c{1.8*}The documented output values and the surrounding prose are unchanged.
Why
Thanks @ramanathan1504 for pointing this out — the earlier revision of this PR rewrote the expected outputs instead, which was the wrong fix.
The period before
*changes which abbreviator is selected.NameAbbreviator.getAbbreviator()routes1.2.*toPatternAbbreviator(one character count per segment,*= keep the rest verbatim) but1.2*toDynamicWordAbbreviator(keep the rightmost N tokens in full).The prose above the table describes the
DynamicWordAbbreviatorsemantics:So the table rows are correct as written — it is the pattern spelling that was wrong. With the period removed, the prose, the pattern and the documented output all agree.
Verification
Measured against the released
log4j-core2.26.1, callingNameAbbreviator.getAbbreviator(precision)directly for logger nameorg.apache.commons.test.Foo:The last three rows are the
PatternAbbreviatorpath and are what the page produced before this change; the first three are theDynamicWordAbbreviatorpath the prose describes.%c{1.1.1.*}is left as-is: all three counts are1, so both abbreviators happen to produceo.a.c.test.Fooand the row is already correct.Scope
Documentation only — no behaviour change. Existing
NameAbbreviatorTestassertions for1.2*/1.3*/1.8*are untouched and still pass.Refs #2522