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

Use private final loggers instead of private static final in Checkstyle codebase #929

Closed
mkordas opened this Issue Apr 14, 2015 · 13 comments

Comments

Projects
None yet
7 participants
@mkordas
Contributor

mkordas commented Apr 14, 2015

Follow-up to discussion in #920

As stated in http://wiki.apache.org/commons/Logging/StaticLog libraries should use private final loggers instead of private static final ones.

  • all loggers should be changed to private final
  • PMD rule ProperLogger should be not violated or suppressed - it states A logger should normally be defined private static final and be associated with the correct class. Private final Log log; is also allowed for rare cases where loggers need to be passed around, with the restriction that the logger needs to be passed into the constructor.
  • PMD rule LoggerIsNotStaticFinal needs to be suppressed
  • ideally we should get rid of all loggers that are unnecessary

@romani romani added the approved label Apr 14, 2015

@rnveach rnveach added the easy label Mar 2, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 18, 2017

Member

@samuel-gu , please help us with this.

Member

romani commented Mar 18, 2017

@samuel-gu , please help us with this.

@samuel-gu

This comment has been minimized.

Show comment
Hide comment
@samuel-gu

samuel-gu Mar 18, 2017

Contributor

I'm on it

Contributor

samuel-gu commented Mar 18, 2017

I'm on it

@samuel-gu

This comment has been minimized.

Show comment
Hide comment
@samuel-gu

samuel-gu Mar 21, 2017

Contributor

@romani @mkordas About 15 files with private static final loggers are input files for tests. They don't have declared constructors. Should I go ahead and change those loggers to private final as well?

Contributor

samuel-gu commented Mar 21, 2017

@romani @mkordas About 15 files with private static final loggers are input files for tests. They don't have declared constructors. Should I go ahead and change those loggers to private final as well?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 21, 2017

Member

About 15 files with private static final loggers are input files for tests.

no, inputs should not be changed, we care only about "main" code

Member

romani commented Mar 21, 2017

About 15 files with private static final loggers are input files for tests.

no, inputs should not be changed, we care only about "main" code

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 28, 2017

Member

@samuel-gu , are going to continue ? If you do not have time now please let us know we could give this issue to smb else

Member

romani commented Apr 28, 2017

@samuel-gu , are going to continue ? If you do not have time now please let us know we could give this issue to smb else

@samuel-gu

This comment has been minimized.

Show comment
Hide comment
@samuel-gu

samuel-gu Apr 28, 2017

Contributor

I was planning of finishing it after finals week. I guess I don't have time now

Contributor

samuel-gu commented Apr 28, 2017

I was planning of finishing it after finals week. I guess I don't have time now

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 29, 2017

Member

@samuel-gu , please define dates when you plan to continue, not let us know till what time keep this on you.

Member

romani commented Apr 29, 2017

@samuel-gu , please define dates when you plan to continue, not let us know till what time keep this on you.

@samuel-gu

This comment has been minimized.

Show comment
Hide comment
@samuel-gu

samuel-gu May 2, 2017

Contributor

Saturday, May 6th

Contributor

samuel-gu commented May 2, 2017

Saturday, May 6th

@crud3

This comment has been minimized.

Show comment
Hide comment
@crud3

crud3 Oct 3, 2017

Contributor

Because this issue seems free/abandoned and the PR #4076 is closed but not merged and marked as "incomplete", I have decided to give it a try.

See PR #5169

Contributor

crud3 commented Oct 3, 2017

Because this issue seems free/abandoned and the PR #4076 is closed but not merged and marked as "incomplete", I have decided to give it a try.

See PR #5169

crud3 added a commit to crud3/checkstyle that referenced this issue Oct 10, 2017

crud3 added a commit to crud3/checkstyle that referenced this issue Oct 10, 2017

crud3 added a commit to crud3/checkstyle that referenced this issue Oct 10, 2017

crud3 added a commit to crud3/checkstyle that referenced this issue Oct 14, 2017

crud3 added a commit to crud3/checkstyle that referenced this issue Oct 15, 2017

crud3 added a commit to crud3/checkstyle that referenced this issue Oct 18, 2017

crud3 added a commit to crud3/checkstyle that referenced this issue Oct 18, 2017

crud3 added a commit to crud3/checkstyle that referenced this issue Oct 19, 2017

crud3 added a commit to crud3/checkstyle that referenced this issue Oct 20, 2017

romani added a commit that referenced this issue Oct 20, 2017

@romani romani added this to the 8.4 milestone Oct 20, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 20, 2017

Member

Fix is merged

Member

romani commented Oct 20, 2017

Fix is merged

@tsjensen

This comment has been minimized.

Show comment
Hide comment
@tsjensen

tsjensen Nov 4, 2017

Contributor

In the Apache Commons Wiki it says:

Just to be completely clear: this only applies to code deployed via a classloader that is shared across multiple "independent" applications. None of this discussion is relevant to normal application code, or to library code that is never deployed into a shared classpath. These categories of code can safely use static references.

Would you mind sharing the scenario you have in mind where Checkstyle is deployed in a shared classloader? This might affect associated libraries also. Thank you!

Contributor

tsjensen commented Nov 4, 2017

In the Apache Commons Wiki it says:

Just to be completely clear: this only applies to code deployed via a classloader that is shared across multiple "independent" applications. None of this discussion is relevant to normal application code, or to library code that is never deployed into a shared classpath. These categories of code can safely use static references.

Would you mind sharing the scenario you have in mind where Checkstyle is deployed in a shared classloader? This might affect associated libraries also. Thank you!

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 4, 2017

Member

I do not have exact example, but as we are library we can appear at any place, user can use us even only for parsing of java code only. We do not know our location, and we want to be less static as possible. To be less static by fields helps in MT mode and improve design, the only expense I see is few extra objects in memory - not a big deal. Logging is not used that much in our library.

If you see scenarios where non static logger is a problem, please share.

Member

romani commented Nov 4, 2017

I do not have exact example, but as we are library we can appear at any place, user can use us even only for parsing of java code only. We do not know our location, and we want to be less static as possible. To be less static by fields helps in MT mode and improve design, the only expense I see is few extra objects in memory - not a big deal. Logging is not used that much in our library.

If you see scenarios where non static logger is a problem, please share.

@tsjensen

This comment has been minimized.

Show comment
Hide comment
@tsjensen

tsjensen Nov 8, 2017

Contributor

Thank you for the explanation!

Contributor

tsjensen commented Nov 8, 2017

Thank you for the explanation!

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment