-
Notifications
You must be signed in to change notification settings - Fork 147
Remove Lombok from several Parameters classes #919
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
Conversation
@@ -23,4 +16,43 @@ public class AppTokenProviderParameters { | |||
public String claims; | |||
/// tenant id | |||
public String tenantId; | |||
|
|||
public AppTokenProviderParameters(Set<String> scopes, String correlationId, String claims, String tenantId) { |
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.
@Avery-Dunn - is there value in reviewing these changes ? I.e. did you de-lombok all of this, in which case it's generaetd code and we can take it at face value?
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 this first PR, yes:
- Helps to understand what Lombok was doing, and what effects removing it will have
- Demonstrates what the Lombok plugin will produce and confirms it's correct
- Shows what still needs to be manually adjusted, mainly the comments used in API docs
Going forward I don't think so. The next PR will affect a lot more classes, but as long as there are no API changes (as confirmed via RevAPI) and tests pass then it shouldn't require any sort of deep review.
@@ -3,15 +3,8 @@ | |||
|
|||
package com.microsoft.aad.msal4j; | |||
|
|||
import lombok.AllArgsConstructor; |
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.
What is the argument for removing lombok? I thought Lombok generates this code at compile time?
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.
Couple things motivated the ask: 1) Lombok needs plugins in IDEs to make the experience reasonable and 2) the debugging experience can be pretty frustrating.
@JonathanGiles any additional thoughts?
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.
Lombok is also not an official Java annotation processor, in the sense that is does not uses public APIs exposed by the Java compiler. This is risky and may be broken at any time. It feels bad to build such critical library code on such a weak foundation, especially when the code being generated by Lombok can easily be written by us (or our IDE) once, without these concerns in perpetuity.
This PR begins the process of removing org.projectlombok.lombok, #909
Lombok is used to generate a lot of boilerplate code, such as getters/setters, builders, and constructures. Removing it all at once would produce a large number of changes which would be hard to review, so this first PR keeps it focused by only removing Lombok from a few similar classes as an example of how follow-up PRs will look.
This PR makes the same basic changes to each file:
@Getter
and@Setter
annotations with the actual getter/setters@Accessors(fluent = true)
annotation, which simply made the generated getter/setters names exactly the same as the fields (as an example, the getter for thescopes
field wasscopes()
instead ofgetScopes()
)@Builder
with an inner builder class@NonNull
with our ownParameterValidationUtils.validateNotNull()
to ensure certain parameters are not nullAnd finally, while removing Lombok we must ensure that no breaking changes are made to any public API. To accomplish that, the RevAPI plugin was used alongside normal testing to ensure the new methods had the same name and the same scope.