Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[OPIK-743] Fix properties naming strategy #1035

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

thiagohora
Copy link
Contributor

Details

Fix properties naming strategy

Issues

OPIK-743

@thiagohora thiagohora requested a review from a team as a code owner January 13, 2025 16:13
@thiagohora thiagohora self-assigned this Jan 13, 2025
andrescrz
andrescrz previously approved these changes Jan 14, 2025
Copy link
Collaborator

@andrescrz andrescrz left a comment

Choose a reason for hiding this comment

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

Just a minor comment, but not a blocker.


import java.time.Instant;
import java.util.List;
import java.util.UUID;

@Data
@Getter
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remind me about this. @Data implements all typical Java methods, such as toString, equals & hascode etc. why are we losing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I actually want to make it immutable, but I can achieve this by making the properties final and avoiding having to add @tostring and @EquaslAndHashCode

Copy link
Collaborator

@andrescrz andrescrz left a comment

Choose a reason for hiding this comment

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

It'd be fine to address the comment in next PRs.

@@ -19,17 +21,20 @@

import static com.comet.opik.api.AutomationRuleEvaluatorLlmAsJudge.LlmAsJudgeCode;

@EqualsAndHashCode(callSuper = true)
@Data
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about this other @Data annotation being removed?

Copy link
Contributor Author

@thiagohora thiagohora Jan 14, 2025

Choose a reason for hiding this comment

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

No, sorry. This has the equals and hash code and is to string because it needs the call to super. So it's okay.

@thiagohora thiagohora merged commit 53fda9f into main Jan 14, 2025
8 checks passed
@thiagohora thiagohora deleted the thiagohora/OPIK-743_fix_property_naming_case branch January 14, 2025 09:03
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants