Fix test for trailingEmptyStrict policy validation#1569
Fix test for trailingEmptyStrict policy validation#1569olabusayoT merged 1 commit intoapache:mainfrom
Conversation
mbeckerle
left a comment
mbeckerle
left a comment
There was a problem hiding this comment.
+1 just suggest adding comments to code explaining the reasoning.
| "Empty trailing optional elements are not allowed when dfdl:separatorSuppressionPolicy='trailingEmptyStrict'" | ||
| ) | ||
| case _ => // ok | ||
| if (checkTrailingOptionalElements(resultOfTry)) { |
There was a problem hiding this comment.
Add comments to explain why you need to check first resultOfTry and then priorResultOfTry. Capture some of the reasoning from our debug session.
There was a problem hiding this comment.
Yeah, I think a comment explaining this is really important.
For example, why do we need to check both resultOfTry AND priorResultOfTry? My intuition is only the most recently parsed thing should matter for trailing empty since we only care about the trailing thing.
Also, why is this only checked for trailingEmptyStrict? Why do we not need to check for other thingslike PositionTrailingLax?
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+1, minor suggests and needs comments and an explanation why this is the right fix.
| } | ||
| } | ||
|
|
||
| private def ParseErrorEmptyTrailingStrict( |
There was a problem hiding this comment.
I'm not sure this should be capitalized, it make its usage look like it's calling an object apply method.
| if (checkTrailingOptionalElements(resultOfTry)) { | ||
| ParseErrorEmptyTrailingStrict(parser, pstate) | ||
| } else if (checkTrailingOptionalElements(priorResultOfTry)) { | ||
| ParseErrorEmptyTrailingStrict(parser, pstate) |
There was a problem hiding this comment.
The blocks of these two if statements are the same, I would suggest just doing something like:
if (checkTrailingOptionalElements(resultOfTry) || checkTrailingOptionalElements(priorResultOfTry) {
parser.PE(...)
}| "Empty trailing optional elements are not allowed when dfdl:separatorSuppressionPolicy='trailingEmptyStrict'" | ||
| ) | ||
| case _ => // ok | ||
| if (checkTrailingOptionalElements(resultOfTry)) { |
There was a problem hiding this comment.
Yeah, I think a comment explaining this is really important.
For example, why do we need to check both resultOfTry AND priorResultOfTry? My intuition is only the most recently parsed thing should matter for trailing empty since we only care about the trailing thing.
Also, why is this only checked for trailingEmptyStrict? Why do we not need to check for other thingslike PositionTrailingLax?
| ParseAttemptStatus.AbsentRep, | ||
| ParseAttemptStatus.EmptyRep | ||
| ).contains(resultToTest) | ||
| } |
There was a problem hiding this comment.
Having a comment here would be helpful to. MIssing and Absent are both types of Failure parse status, so I would think that indicates error. But I guess because these are optional trailing elements those are not considered errors?
olabusayoT
force-pushed
the
daf-2217-update-checks
branch
2 times, most recently
from
d5efb54 to
b3d17c7
Compare
- Resolved issues with trailingEmptyStrict violation when maxOccurs is '3' by improving checks for empty trailing optional elements. - Refactored code to introduce helper functions for condition checks with appropriate comments DAFFODIL-2217
olabusayoT
force-pushed
the
daf-2217-update-checks
branch
from
b3d17c7 to
e7e763f
Compare
4 participants
DAFFODIL-2217