-
Notifications
You must be signed in to change notification settings - Fork 9
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
Support weak typed data attributes, and rename related methods #189
Support weak typed data attributes, and rename related methods #189
Conversation
b64802a
to
5177e98
Compare
Codecov Report
@@ Coverage Diff @@
## main #189 +/- ##
============================================
+ Coverage 69.37% 69.99% +0.61%
- Complexity 349 355 +6
============================================
Files 58 59 +1
Lines 1476 1513 +37
Branches 134 135 +1
============================================
+ Hits 1024 1059 +35
- Misses 379 381 +2
Partials 73 73
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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, just some minor comments
dataAttributeField.getDataAttributeType()) | ||
); | ||
for (final DataAttributeDef dataAttributeField : fields) { | ||
if (!dataAttributeField.isPrefix()) { |
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.
Nit: swap the order of the if-else will make it slightly better to read:
if (dataAttributeField.isPrefix()) {
else {}
.build(); | ||
} | ||
|
||
public static DataAttributeDef createByPrefix(final Class dataType, final String key) { |
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.
nit: key ==> keyPrefix
Also it will better to provide some comments to it. As it's not straightforward to know why this is needed.
This MR did: