-
Notifications
You must be signed in to change notification settings - Fork 927
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
Add lint checks #561
Add lint checks #561
Conversation
2da5a51
to
367be43
Compare
|
||
/** | ||
* Contains the claim name for all Public claims. | ||
*/ | ||
public interface PublicClaims { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for this class to be in the impl
(internal package) ?
I see some other classes using public
in this package, this is something to consider change in future.
This package should also be removed from module-info (java 9+).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My assumption on this class being internal is to provide less API surface area for the consumer. But as you mentioned having it as public negates the effort and we don't have any other option currently since the package structure is that way.
For Java 9+, if the project is using the module feature, then they won't be able to import this package as it can be seen here - https://github.com/auth0/java-jwt/blob/v4-dev/lib/src/main/java/module-info.java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next major version is a big opportunity to remove public from interfaces/classes in impl package.
Module system is under utilized in Java, even if you using latest Java 18 everyone still relying on classloader.
Lets make sure this get reviewed before release, ideally should have none.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree @panga, I have created an internal ticket [SDK-3226] to assigned it to this major. Since @jimmyjames has more context over this library, we should decide on this once he is back.
|
||
checkstyle { | ||
toolVersion '10.0' | ||
checkstyleTest.enabled = false //We are disabling lint checks for tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why disabling tests? what's the side-effect on enabling there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are disabling this for tests since there 500+ errors in our test code base as well. I thought we could refactor it in a later release. What would be your suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍
Context
We have added Lint checks to ensure consistent coding style across our repository
Changes
References
https://checkstyle.sourceforge.io/
Testing