Skip to content

Add System.Logger to logger hints and code generation; minor new features#8253

Merged
matthiasblaesing merged 1 commit intoapache:masterfrom
errael:SystemLogger
Mar 29, 2025
Merged

Add System.Logger to logger hints and code generation; minor new features#8253
matthiasblaesing merged 1 commit intoapache:masterfrom
errael:SystemLogger

Conversation

@errael
Copy link
Copy Markdown
Contributor

@errael errael commented Feb 16, 2025 •
edited by matthiasblaesing
Loading

This default is also used by Generate > Logger.... Addresses some issues in raised in #8240.
New feature Use existing declared Logger. Fix Logging hints to handle System.Logger.
Add "Put string concatenation in lambda" hint fix. Adjust tests; new tests.

trycatch

@errael
Add System.Logger to logger hints and code generation; minor new fe…
b5ee36a 
…atures

In `Surround with try-catch` hint add/default to `System.Logger`.
This default is also used by `Generate > Logger...`.
Addresses some issues in raised in apache#8240.
New feature: `Use existing declared Logger`.
New feature: hint fix `convert string concatenation to Supplier<String>`
Fix "Logging" hints to handle System.Logger.
@errael errael marked this pull request as draft February 16, 2025 22:32
@errael
Copy link
Copy Markdown
Contributor Author

errael commented Feb 16, 2025 •
edited
Loading

Notes/issues/questions about this PR.

"java.lang.System.Logger" is used directly because:

System.Logger.class.getName() is "java.lang.System$Logger", note '$',
and "...getQualifiedName().contentEquals" fails, so use litteral class name.

Generate > Logger... requires knowing which kind of logger. That information is only specified in Hints > ErrorFixes > SurroundWithTryCatch. It's weird to be looking into the hints this way, but otherwise some entirely new UI is required. The PR uses FileHintPreferences to grab this info, I've never used it before so this code should be carefully reviewed.
See org.netbeans.modules.java.editor.codegen.LoggerGenerator.isUseSystemLogger

Using System.Logger with code that currently uses java.util.logging works seamlessly when using the default System.LoggerFinder. Agree?

// NOI18N doesn't seem to be used consistently; assuming it's no longer required.

@errael errael mentioned this pull request Feb 16, 2025
Open
@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests hints labels Feb 16, 2025
@errael errael force-pushed the SystemLogger branch 2 times, most recently from b25ef0a to 4cdd426 Compare February 18, 2025 20:50
@errael errael marked this pull request as ready for review February 18, 2025 20:54
@errael errael marked this pull request as draft February 18, 2025 23:43
@errael

This comment was marked as outdated.

Sign in to view
@errael errael marked this pull request as ready for review February 19, 2025 01:44
@errael errael force-pushed the SystemLogger branch 3 times, most recently from 5d06f2f to 7d026d2 Compare February 20, 2025 01:09
@errael
Copy link
Copy Markdown
Contributor Author

errael commented Feb 26, 2025

@lahodaj The code for this PR is all about Hints and Generate>Logger; java.compiler and NB's ...api.java.source stuff (relatively unfamiliar to me). The small OP and 1st comment mention possible issues; any comments about these appreciated. (of course a review would be terrific)

matthiasblaesing
matthiasblaesing reviewed Mar 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

This seems to do what it promises, thank you.

I left a few comments inline.

Comment thread java/java.hints/src/org/netbeans/modules/java/hints/MultipleLoggers.java Outdated
Comment thread ...s/test/unit/src/org/netbeans/modules/java/hints/errors/MagicSurroundWithTryCatchFixTest.java Outdated
Comment thread java/java.hints/src/org/netbeans/modules/java/hints/jackpot/hintsimpl/LoggerStringConcat.java Outdated
Comment thread java/java.hints/src/org/netbeans/modules/java/hints/errors/MagicSurroundWithTryCatchFix.java
Comment thread java/java.editor/src/org/netbeans/modules/java/editor/codegen/LoggerGenerator.java
@errael
Copy link
Copy Markdown
Contributor Author

errael commented Mar 28, 2025

Pushed fixes for everything but #8253 (comment), which is still under discussion.

@errael errael requested a review from matthiasblaesing March 28, 2025 00:25
@errael
Copy link
Copy Markdown
Contributor Author

errael commented Mar 28, 2025

Pushed fix for #8253 (comment) as well.

matthiasblaesing
matthiasblaesing reviewed Mar 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Thank you for the update. Looks good to me. I left two followup comments inline and please update the commit message to have a summary in the first line and a an empty line as separator to the complete message.

Comment thread java/java.hints/src/org/netbeans/modules/java/hints/errors/MagicSurroundWithTryCatchFix.java Outdated
Comment thread java/java.editor/src/org/netbeans/modules/java/editor/codegen/LoggerGenerator.java
@errael errael changed the title In Surround with try-catch hint add/default to System.Logger. Add System.Logger to logger hints and code generation; minor new features Mar 28, 2025
@errael
Copy link
Copy Markdown
Contributor Author

errael commented Mar 29, 2025 •
edited
Loading

using "LOG" violates recommended java style

akin to the tab-vs-space or "the right IDE" wars

Sure. Though in this case, with both original Sun java and current Google guidelines, there's considerable weight behind the "shouldn't be all caps" POV.

@errael errael requested a review from matthiasblaesing March 29, 2025 00:02
@errael
Copy link
Copy Markdown
Contributor Author

errael commented Mar 29, 2025

There's a test failure. View Job Summary gives

Java Hints batch1 on Linux/JDK 17 summary

1463 passed, 1 failed

Test failures:
  testOrdering1: org.netbeans.modules.java.hints.FieldForUnusedParamTest
  ...

Doesn't seem related to this PR. OK to ignore?

@matthiasblaesing matthiasblaesing added this to the NB26 milestone Mar 29, 2025
@matthiasblaesing
Copy link
Copy Markdown
Contributor

matthiasblaesing commented Mar 29, 2025

@errael yes this is one of the flaky tests. Tests are green, so lets get this in. Thank you.

@matthiasblaesing matthiasblaesing merged commit e2dce56 into apache:master Mar 29, 2025
36 checks passed
@errael errael deleted the SystemLogger branch March 29, 2025 20:01
@neilcsmith-net neilcsmith-net mentioned this pull request Jun 11, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthiasblaesing matthiasblaesing Awaiting requested review from matthiasblaesing

Assignees

No one assigned

Labels

hints Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests

Projects

None yet

Milestone

NB26

Development

Successfully merging this pull request may close these issues.

3 participants

@errael @matthiasblaesing @mbien