-
Notifications
You must be signed in to change notification settings - Fork 25
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
Automatically set units and precision in rules #391
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #391 +/- ##
=========================================
Coverage 71.26% 71.26%
Complexity 6710 6710
=========================================
Files 657 657
Lines 22245 22245
Branches 3572 3572
=========================================
Hits 15853 15853
Misses 4698 4698
Partials 1694 1694
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
CAttribute precision = template.getDefinition().itemAtPath(pathOfParent + "/precision"); | ||
|
||
if (units.getChildren().size() != 1 || precision.getChildren().size() != 1) | ||
return result; // Only fix if there is 1 unit and 1 precision, don't assume anything otherwise |
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.
You could split these two checks for more auto-fixing.
Also note that precision.getChildren() contains intervals, and you cannot use those.
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.
and you cannot use those.
Not exactly sure what you mean, am I not allowed to do a precision.getChildren().size() != 1
?
|
||
// Fix units | ||
CString cString = (CString) units.getChildren().get(0); | ||
String assumedUnitValue = cString.getAssumedValue(); |
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.
assumed value won't work, that's an explicitly set thing in ADL. You need the actual first+only constraint here, need to check if it's not a regular expression (starts+ends with / or ^), and set it.
|
||
// Fix precision | ||
CInteger cInteger = (CInteger) units.getChildren().get(0); | ||
long assumedPrecisionValue = cInteger.getAssumedValue(); |
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.
assumed value won't work, that's an explicitly set thing in ADL. You need the actual first+only constraint here. This constraint will be an Interval<Long>
. If interval.isUpperBounded()
, you can set precision to -1.
Otherwise, if interval.isUpperIncluded()
set it to interval.upper
else set it to interval.upper -1
.
|
||
private static Map<String, Object> fixDvQuantity(Object rmObject, Archetype archetype, String pathOfParent) { | ||
try { | ||
Map<String, Object> result = new HashMap<>(); |
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.
Two more todo's:
- check if actually the magnitude is being set (see the pathOfParent.endsWith contruction elsewhere)
- if units or precision already have a value, should they be set? You probably do not want to override a value set manually or with a different rule elsewhere?
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.
check if actually the magnitude is being set (see the pathOfParent.endsWith contruction elsewhere)
I only see .../value
in the pathOfParent
when these quantities are being set by rule evaluation. Not sure how that would work then, or should I expect a .../value/magnitude
path or something.
9c8fc92
to
f9d219f
Compare
f9d219f
to
2ab72e3
Compare
2ab72e3
to
8aec62b
Compare
For some testing on mobile implementation for now.
No tests yet