Add accessors for MergeState and SegmentCommitInfo's public fields #14989
Conversation
Signed-off-by: Liyun Xiu <[email protected]>
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
Signed-off-by: Liyun Xiu <[email protected]>
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
chishui
changed the title
| /** Returns whether the index needs to be sorted. */ | ||
| public boolean getNeedsIndexSort() { | ||
| return needsIndexSort; | ||
| } |
There was a problem hiding this comment.
Adding a getter to a public field is senseless java verbosity: it adds no value. The field is public: just access the field directly.
There was a problem hiding this comment.
Thanks for reviewing. I fully understand that you can just use public fields but that's not the point. Like what I mentioned in the description, it provides better testability, we can easily mock a member function but we can't mock a field access.
Apart from that, it's not good practice to have public fields as it doesn't provide good encapsulation. It's just the fields are already public and for backward compatibility, we can't change them to private.
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! |
Description
Found some classes don't have good encapsulation and doesn't offer accessors for its fields. It makes mocking MergeState in unit tests difficult.
We can't modify the access level for these fields, so I add getter functions for all these public fields.