Skip to content

[#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

Merged

Conversation

724thomas
Copy link
Contributor

What changes were proposed in this pull request?

This pull request introduces the following key changes:

  • Removal of '@Getter' and '@EqualsAndHashCode' Annotations
  • Addition of Getter Methods
  • Implementation of 'equals' and 'hashCode' Methods
  • Custom 'toString' Method Implementations: The format adopted is "METHOD NAME " + variable.

Special Handling:

  • Temporary Renaming for Windows Compatibility: Encountered an issue with the "aux" directory, which is a reserved name in Windows environment. This directory was temporarily renamed during development to circumvent this issue.

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

@qqqttt123
Copy link
Contributor

qqqttt123 commented Jan 7, 2024

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.

@724thomas
Copy link
Contributor Author

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.

Added Java doc comments for all methods. Also, removed annotations in SchemaChange.java. I found out that I've missed these out.

@724thomas
Copy link
Contributor Author

Also, applied spotless code formatting

qqqttt123
qqqttt123 previously approved these changes Jan 7, 2024
Copy link
Contributor

@qqqttt123 qqqttt123 left a comment

Choose a reason for hiding this comment

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

LGTM, cc @mchades .

@jerryshao
Copy link
Contributor

@724thomas Would you please update the api/build.gradle.kts file to lombok dependency? Thanks.

@mchades
Copy link
Contributor

mchades commented Jan 8, 2024

Overall, LGTM, but the comment above needs to be addressed.

@jerryshao
Copy link
Contributor

Special Handling:

Temporary Renaming for Windows Compatibility: Encountered an issue with the "aux" directory, which is a reserved name > in Windows environment. This directory was temporarily renamed during development to circumvent this issue.

@724thomas We have fixed this issue, can you please retest it, thanks.

@724thomas
Copy link
Contributor Author

724thomas commented Jan 8, 2024

Special Handling:
Temporary Renaming for Windows Compatibility: Encountered an issue with the "aux" directory, which is a reserved name > in Windows environment. This directory was temporarily renamed during development to circumvent this issue.

@724thomas We have fixed this issue, can you please retest it, thanks.

@jerryshao
Thank you for the update. I have retested the fix for the "aux" directory issue on Windows, and I can confirm that it now functions correctly.

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.

@qqqttt123
Copy link
Contributor

Special Handling:
Temporary Renaming for Windows Compatibility: Encountered an issue with the "aux" directory, which is a reserved name > in Windows environment. This directory was temporarily renamed during development to circumvent this issue.

@724thomas We have fixed this issue, can you please retest it, thanks.

@jerryshao Thank you for the update. I have retested the fix for the "aux" directory issue on Windows, and I can confirm that it now functions correctly.

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?

@jerryshao
Copy link
Contributor

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.
@724thomas
Copy link
Contributor Author

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.

Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

LGTM

@jerryshao jerryshao merged commit 7086eca into apache:main Jan 8, 2024
@qqqttt123
Copy link
Contributor

Thanks @724thomas for your contribution. Thanks @jerryshao @mchades for review. Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Remove Lombok annotation from API module
4 participants