Skip to content

Conversation

@java-coding-prodigy
Copy link
Contributor

Fixes #365

@java-coding-prodigy java-coding-prodigy requested review from a team as code owners February 6, 2022 08:36
Tais993
Tais993 previously approved these changes Feb 6, 2022
@krankkkk
Copy link
Contributor

krankkkk commented Feb 6, 2022

As said in the ticket, no reason for the field. So I would close this

Copy link
Contributor

@marko-radosavljevic marko-radosavljevic left a comment

Choose a reason for hiding this comment

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

I like consistency, so the changes make sense to me. Please check if there is any other place in the codebase where similar thing occurs, so we can align.

I will temporarily put a hold on this PR, because code owner (@krankkkk) disagrees with the changes. ^^

@java-coding-prodigy
Copy link
Contributor Author

Please check if there is any other place in the codebase where similar thing occurs, so we can align.

If I find something will I add that to this PR itself, or another one?

@Tais993
Copy link
Member

Tais993 commented Feb 7, 2022

This PR is fine

@java-coding-prodigy
Copy link
Contributor Author

SonarLint is unhappy that SpringBootServletInitializer(super class of Application in logviewer) has a protected field with the name logger. What do we do about this?

@Tais993
Copy link
Member

Tais993 commented Feb 7, 2022

Time to remove sonarlint since it's giving me mental issues

All loggers should be private, so change that

@java-coding-prodigy
Copy link
Contributor Author

All loggers should be private, so change that

Um, I cannot do that, it is a class from Spring that is causing this issue. Note that the logger there is of class org.apache.commons.logging.Log and not an SLF4J logger

@Tais993
Copy link
Member

Tais993 commented Feb 7, 2022

Rename our logger to log ig?

Just Sonarlint being stupid again

@java-coding-prodigy
Copy link
Contributor Author

Just Sonarlint being stupid again

I could suppress that warning, imo that would be better.

@Tais993
Copy link
Member

Tais993 commented Feb 7, 2022

Then I'd do that

@marko-radosavljevic marko-radosavljevic dismissed their stale review February 23, 2022 13:32

seems like we agreed on how to proceed with this ^^

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Zabuzard Zabuzard merged commit aa68787 into develop Feb 23, 2022
@Zabuzard Zabuzard deleted the add_logger_field branch February 23, 2022 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generation of logger inside methods

7 participants