-
Notifications
You must be signed in to change notification settings - Fork 581
[#1220] improvement: Remove Lombok annotation from API module #1359
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
[#1220] improvement: Remove Lombok annotation from API module #1359
Conversation
Added get, equals, hashcode, toString method Note that I have created toString method with style based on other methods.
Added get, equals, hashcode, toString method Note that I have created toString method with style based on other methods.
Added get, equals, hashcode, toString method Note that I have created toString method with style based on other methods.
Could you add new Java doc comments for new method? For Gravitino, if a method or a class is public, we would better add Java doc for them. |
…ts in SchemaChange.java. Added get, equals, hashcode, toString method and Java doc comments for them Note that I have created toString method with style based on other methods.
Added Java doc comments for get, equals, hashcode, tostring methods.
Added Java doc comments for get, equals, hashcode, tostring methods.
Added Java doc comments for get, equals, hashcode, tostring methods.
Added Java doc comments for all methods. Also, removed annotations in SchemaChange.java. I found out that I've missed these out. |
…licity' into #1220_Remove_Annotations_for_explicity
Also, applied spotless code formatting |
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.
LGTM, cc @mchades .
@724thomas Would you please update the |
Overall, LGTM, but the comment above needs to be addressed. |
@724thomas We have fixed this issue, can you please retest it, thanks. |
@jerryshao Regarding the request to "update the api/build.gradle.kts file for the Lombok dependency", I would appreciate some additional clarification. From my understanding, this could involve either upgrading the version of Lombok from 1.18.20 to 1.18.30 or changing the dependency configuration from "compileOnly" to "implementation". Could you please specify which of these actions is required? I want to ensure that I accurately understand and implement the necessary changes to project’s Lombok configuration. Your guidance on this matter would be highly valuable. |
Could you remove the lombok dependency? |
Sorry for my confusion, just removing the lombok dependencies is enough. |
This commit removes the Lombok dependency from the project, as specified in the requirements. All references to Lombok annotations have been replaced with standard Java implementations. Ensured the project builds successfully and all tests pass after the removal.
Last commit removes the Lombok dependency from the project, as specified in the requirements. Ensured the project builds successfully and all tests pass after the removal. |
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.
LGTM
Thanks @724thomas for your contribution. Thanks @jerryshao @mchades for review. Merged to master. |
What changes were proposed in this pull request?
This pull request introduces the following key changes:
Special Handling:
Justification for 'toString' Method Format
The chosen format for the 'toString' method, "METHOD NAME " + variable", is in line with the existing style observed in other classes within the project.
Feedback on the changes, particularly the 'toString' method's format and the temporary workaround for the "aux" directory, is welcomed to ensure alignment with the project's coding conventions. If there are any concerns or preferences for a different approach, please let me know for necessary adjustments.
Why are the changes needed?
Fix: #1220
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
N/A