Skip to content

Add missing FileLocationPatternConverter#requiresLocation() (#2781) #2782

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 6, 2024
Merged

Add missing FileLocationPatternConverter#requiresLocation() (#2781) #2782

merged 4 commits into from
Aug 6, 2024

Conversation

@dedeibel
Copy link
Contributor

@dedeibel dedeibel commented Jul 30, 2024

Fix for #2781

@ppkarwasz
Copy link
Contributor

ppkarwasz commented Jul 30, 2024

@dedeibel,

Nice catch. Could you also add a unit test that verifies that the pattern converters %L, %l, %F, %C and %M implement LocationAware and their requiresLocation() method returns true?

@github-actions
Copy link

github-actions bot commented Jul 30, 2024 •
edited
Loading

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 ❌ Build Scan PUBLISHED
build-ubuntu-latest clean install 3.9.8 ✅ Build Scan PUBLISHED
build-windows-latest clean install 3.9.8 ✅ Build Scan PUBLISHED
Generated by gradle/develocity-actions

@vy
Copy link
Member

vy commented Jul 31, 2024

@dedeibel, thanks so much for the fix. Would you mind porting the fix to the 2.x branch with the changes @ppkarwasz requested in a separate PR, please?

@dedeibel
Copy link
Contributor Author

dedeibel commented Jul 31, 2024

@dedeibel,

Nice catch. Could you also add a unit test that verifies that the pattern converters %L, %l, %F, %C and %M implement LocationAware and their requiresLocation() method returns true?

@ppkarwasz Thanks. I will try but please bear with me hat it might take some time to find my way around the tests.

@vy Yes I will. But please give me a some time.

@vy
Copy link
Member

vy commented Jul 31, 2024

@dedeibel, no worries, take your time. Feel free to reach out to us.

Note that we'd prefer fixing 2.x first, since Log4j 2 is heavily used and there is no Log4j 3 release date on the horizon.

@dedeibel dedeibel mentioned this pull request Aug 4, 2024
Merged
@dedeibel dedeibel marked this pull request as draft August 4, 2024 13:15
@dedeibel dedeibel force-pushed the fix/file-location-pattern-converter-missing-location-requirement branch from 92be49e to 3470919 Compare August 4, 2024 14:03
@dedeibel dedeibel marked this pull request as ready for review August 4, 2024 14:11
vy added 2 commits August 4, 2024 16:46
@vy
Remove changelog
1ad1e6c 
This is effectively port of a fix from `2.x`, no need for a changelog entry.
@vy
Make tests package private
7652286
vy
vy approved these changes Aug 4, 2024
View reviewed changes
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

@dedeibel, good job. 💯

@ppkarwasz, as in #2795, please go ahead and [squash] merge this PR, if you also agree with the changes.

@vy vy changed the title Add requiresLocation method to FileLocationPatternConverter (#2781) Add missing FileLocationPatternConverter#requiresLocation() (#2781) Aug 4, 2024
@vy vy added layouts Affects one or more Layout plugins async Affects asynchronous loggers or appenders labels Aug 4, 2024
@vy vy added this to the 3.x milestone Aug 4, 2024
@vy vy merged commit 24618b8 into apache:main Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vy vy vy approved these changes

Assignees

No one assigned

Labels

async Affects asynchronous loggers or appenders layouts Affects one or more Layout plugins

Projects

None yet

Milestone

3.x

Development

Successfully merging this pull request may close these issues.

3 participants

@dedeibel @ppkarwasz @vy