Skip to content

Comments

CustomCheckStyleRules: Good Logging Practice#3359

Merged
mssfang merged 42 commits intoAzure:masterfrom
mssfang:CheckStyleRule-GoodLoggingPractice
Aug 27, 2019
Merged

CustomCheckStyleRules: Good Logging Practice#3359
mssfang merged 42 commits intoAzure:masterfrom
mssfang:CheckStyleRule-GoodLoggingPractice

Conversation

@mssfang
Copy link
Contributor

@mssfang mssfang commented Apr 12, 2019

Support Good Logging Practice

Good Logging Practice
1) ClientLogger in public API should all named 'logger', public API classes are those classes that are declared
as public and that do not exist in an implementation package or subpackage.
2) ClientLogger should be non-static instance logger
3) Should not use any external logger class, only use ClientLogger. No slf4j, log4j, or other logging imports are allowed.
'System.out' and 'System.err' is not allowed as well.
4) All classes should use ClientLogger as logger only but except ClientLogger itself
fixes #3309

@mssfang mssfang added Client This issue points to a problem in the data-plane of the library. Java Source Code Rules labels Apr 12, 2019
@mssfang mssfang self-assigned this Apr 12, 2019
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@mssfang mssfang requested a review from srnagar August 19, 2019 21:51
@mssfang mssfang requested a review from conniey August 20, 2019 00:49
@mssfang mssfang requested a review from JonathanGiles August 21, 2019 05:05
<!-- Azure Core Test -->
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files=".*[/\\]core[/\\]test[/\\].*\.java"/>
<!-- Public API already released -->
<suppress checks="com.azure.tools.checkstyle.checks.GoodLoggingCheck" files="CertificateUtil.java"/>
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why we added this suppression? It is in an implementation package, and the logger itself is private. 🤔
https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/identity/azure-identity/src/main/java/com/azure/identity/implementation/util/CertificateUtil.java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to use logger in static method, the logger has to be static. Not a problem of private, the methods of the classes are public static methods. Can't change these method.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the API is public, but we could remove the logger and it wouldn't break this API. So I was wondering about the "Public API already release" reason... since it doesn't match up.

shafang and others added 4 commits August 27, 2019 14:15
@mssfang mssfang requested a review from conniey August 27, 2019 22:16
…yle/checks/GoodLoggingCheck.java


wording

Co-Authored-By: Connie Yau <conniey@microsoft.com>
@conniey conniey self-requested a review August 27, 2019 22:54
@mssfang mssfang merged commit ed9d8b6 into Azure:master Aug 27, 2019
@mssfang mssfang deleted the CheckStyleRule-GoodLoggingPractice branch August 27, 2019 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. Java Source Code Rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CheckStyle: Good Logging Practices (9)

5 participants