-
Notifications
You must be signed in to change notification settings - Fork 72
Add warning for ock=parsed without ssp=anyEmpty
#1613
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
base: main
Are you sure you want to change the base?
Conversation
…SuppressionPolicy=anyEmpty` - Introduced `TestImplicitvParsed` including TDML tests for `occursCountKind=parsed` and `occursCountKind=implicit`. - Added "DiscouragedOCKParsedWithoutSSPAnyEmpty" warning to enforce `separatorSuppressionPolicy="anyEmpty"` for `occursCountKind=parsed`. - Implicit with SSP=trailingEmpty works as expected, with it looking for the prefix / for absent arre1 in e2 in the added implicitvparsed.tdml as per the spec (Table 47: RepDef(min) ~ Rep(max - min), which means RepDef(1) followed by Rep(2), so it expects the separator for the last empty arre1 element), so the right thing to do if we don't want the empty trailing / is to use anyEmpty - Updated test cases to align with these changes. DAFFODIL-2801
30ae7db to
9f95a11
Compare
stevedlawrence
left a comment
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.
+1, minor suggestions
| .map(_.asInstanceOf[ElementBase]) | ||
|
|
||
| val invalidChild = validChildren.find(e => { | ||
| if (hasSeparator) { |
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.
I think we can do this hasSeparator check earlier. If the sequence doesn't have a separator, then we can skip looking at the children which could save some time.
| e.occursCountKind match { | ||
| case Parsed => | ||
| separatorSuppressionPolicy match { | ||
| case SeparatorSuppressionPolicy.AnyEmpty => false |
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.
We can also check SSP earlier. If SSP is AnyEmpty then we don't need to look at the children at all, also saving some time.
So this might want to become something like:
if (hasSeparator || separatorSupressionPolicy ne AnyEmpty) {
// look at group members for OCK=parsed elements
}| }.value | ||
|
|
||
| lazy val checkValidityOccursCountKind: Unit = { | ||
| val validChildren: Seq[ElementBase] = |
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.
Instead of doing a filter and map, I would suggest doing a find, that would avoid allocating another sequence, e.g.:
val optInvalidChild = groupMembers.find { m
m match {
case e: ElementBase => e.occursCountKind == parsed
case _ => false
}
}
if (optInvalidChild.defined) {
... // SDE
}
| "Member of a sequence with dfdl:occursCountKind='parsed' must have dfdl:separatorSuppressionPolicy='anyEmpty'. " + | ||
| "This may be changed to an error in a future version of Daffodil." |
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.
This almost makes it sound like the element must have SSP=anyEmpty, but it's really the containing sequence. Maybe something instead like:
Member of a sequence with dfdl:occursCountKind='parsed' must have a containing sequence with dfdl:separatorSuppressionPolicy='anyEmpty', but was " + separatorSuppressionPolicy + ". This may be changed to an error ..."
Also, I'm curious what our behavior is if someone does use OCK=parsed with SSP != anyEmpty. Is the behavior well defined and we essentially assume SSP=anyEmpty? for the parsed element? Things seem to work, or we do we just not have any tests that actually care about this combination or properties?
| <xs:enumeration value="deprecatedRelativeSchemaLocation" /> | ||
| <xs:enumeration value="discouragedDiscriminatorPlacement" /> | ||
| <xs:enumeration value="discouragedAssertPlacement" /> | ||
| <xs:enumeration value="discouragedOCKParsedWithoutSSPAnyEmpty" /> |
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.
I dont' think we should say "discouraged". The discouraged things are legal, they just don't give the behavior a user usually expects.
This is straight up illegal and we are only temprarily allowing it for backwards compatability. Maybe instead something like "separatorSuppressionPolicyError". That's also a bit generic so if there are any other similar disallowed issues with separatorSuppressionPolciy we can use the same warn id. We probably want to avoid id's that are too specific.
| <xs:element name="field2" type="xs:string" minOccurs="0" /> | ||
| <xs:element name="field3" type="xs:string" minOccurs="0" /> | ||
| <xs:element name="groupOfFields" minOccurs="1" maxOccurs="3" dfdl:occursCountKind="parsed"> | ||
| <xs:element name="groupOfFields" minOccurs="1" maxOccurs="3"> |
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.
This changes both SSP and OCK. Why change both? Could we jsut change SSP to anyEmpty (which should work with OKC=parsed) or change OCK=implicit with the existing SSP? I'm not sure what the test is trying to test.
TestImplicitvParsedincluding TDML tests foroccursCountKind=parsedandoccursCountKind=implicit.separatorSuppressionPolicy="anyEmpty"foroccursCountKind=parsed.DAFFODIL-2801