-
Notifications
You must be signed in to change notification settings - Fork 827
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
Miscellaneous refactors #2608
Miscellaneous refactors #2608
Conversation
peterhaochen47
commented
Nov 17, 2023
- particularly codes related to Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider #2590
- by using lombok to "autogenerate" getters - this lombok pattern is already used elsewhere in our codebase
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/186506736 The labels on this github issue will be updated when the story is started. |
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.
Great improvements.
|
||
import java.lang.reflect.ParameterizedType; | ||
import java.net.URL; | ||
import java.util.List; | ||
import java.util.Objects; | ||
|
||
@Getter |
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.
Nice! I'm a big fan of Lombok.
|
||
import java.lang.reflect.ParameterizedType; | ||
import java.net.URL; | ||
import java.util.List; | ||
import java.util.Objects; | ||
|
||
@Getter |
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 am not really a fan of converting existing class to use Lombok. To me, it is most useful for new code. Regardless, if you wanted to change it to use Lombok, why stop at Getter? Why not also use @Setter?
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.
For the Setters, our existing implementations of the setters in this class is not "standard", e.g.:
public T setTokenKeyUrl(URL tokenKeyUrl) {
this.tokenKeyUrl = tokenKeyUrl;
return (T) this;
}
So I can't use the lombok setter without more code changes. It can be done though, just leaving it aside for now.
As of the motivation of this refactor - it's mainly sort of a response to this prior discussion with Bruce. With this PR, I'm making the point that the "isSomeBoolean" getter pattern is now a common pattern, since it's what lombok would generate, though it might not be necessarily grammatically correct.
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 see.
sonar happy, I am happy https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2608 |