-
Notifications
You must be signed in to change notification settings - Fork 30
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
Address code smells in DataGenerator #1076
Conversation
- Defined constants for reused literals - Mark unmodified fields final - Add a private constructor to hide the implicit public constructor Signed-off-by: Peter Nied <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1076 +/- ##
=========================================
Coverage 80.19% 80.19%
- Complexity 2863 2866 +3
=========================================
Files 383 383
Lines 14333 14337 +4
Branches 988 988
=========================================
+ Hits 11494 11498 +4
Misses 2245 2245
Partials 594 594
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
UtilityClass is one annotation I'd rather us not use. It's value is marginal and it requires me to review the entire class/file, which is especially scary when looking at a diff since a reviewer won't see the whole file by default.
Signed-off-by: Peter Nied <[email protected]>
Thanks for the review, I have addressed this comment. |
import static org.opensearch.migrations.data.FieldBuilders.GEO_POINT; | ||
import static org.opensearch.migrations.data.FieldBuilders.INTEGER; | ||
import static org.opensearch.migrations.data.FieldBuilders.LONG; |
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.
Let's continue to not allow static imports outside of test code (this is in src, not testFixture)
Adding the classname will give names like "INTEGER" some more context and clarity
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.
Thanks for the feedback, I don't agree with the style preference. I would prefer to merge this change as is.
I've created MIGRATIONS-2141 [1] to automate enforcement of style issues, lets use tooling to align our preferences.
Description
Address code smells in DataGenerator
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.