Address some unexpected warnings#1305
Conversation
mbeckerle
left a comment
mbeckerle
left a comment
There was a problem hiding this comment.
+1 with a comment about improving this whole FacetsContradict warning set (which is probably a separate issue)
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+1, looks good, minor comments
| <xs:annotation> | ||
| <xs:appinfo source="http://www.ogf.org/dfdl/dfdl-1.0/" /> | ||
| </xs:annotation> | ||
| </xs:element> |
There was a problem hiding this comment.
I'm not sure I understand this change. The description for the test using the element says
A warning message should be generated - use of binaryNumperRep 'binary' with representation 'text'
But the representation is binary. The test doesn't expect a specific warning message, so I wonder if the intention was to get a warning about "property binaryNumberRep was ignored" or something, and it just happened to get some other warning?
Looking at the git log this was added in 2012, so it's hard to say. But I'd suggest we change this to binaryNumberRep="text" and expect a warning about an ignored property
There was a problem hiding this comment.
I think you meant change representation to text, when we do, we get an error about representation text not being allowed for lengthkind implicit. Changing the lengthKind to explicit with length 4 bytes, we get the parse error "Unable to parse xs:int from text: (NUL)(NUL)(NUL)%". No warning either ways.
I did find this in the spec, so maybe the warning is optional?
Property is not applicable because of another DFDL property setting.
Warning (optional). Example is dfdl:binaryNumberRep when dfdl:representation is text.
There was a problem hiding this comment.
The "property not applicable" is a type of warning that we do generate (though our warning is a little less helpful, just saying a property is ignored and leave it up to the user to figure out why).
It's possible the warning is being hidden because of the parse error/test failure? If you change documentPart type="text" and give it a 4-digit text string so the test passes do you see the warning?
I've tested this simple schema and I do get the warning:
<element name="foo" type="xs:string" dfdl:length="1" dfdl:lengthKind="explicit" dfdl:binaryNumberRep="binary" />
Note that it looks like this test was created a long time ago just to test that we could create and expect warnings back when that was a new thing. I think it's well established that that works now. We also have tests specifically about ignore warnings (e.g. ignoredPropertiesWarning in PropertySyntax.tdml. So this test may not even be needed anymore and could maybe be removed. Unless there is value in having a test explicitly checking for binaryNumberRep being ignored. I don't think that's a useless test, but I also don't think it's a super important test to have.
There was a problem hiding this comment.
Ah yes, I get the warning now. Many thanks
| dfdl:lengthKind="explicit" dfdl:length="{ 3 }" dfdl:lengthUnits="bytes" dfdl:binaryCalendarRep="bcd"/> | ||
| <xs:element name="dateBinBCD7" type="xs:date" dfdl:calendarPattern="eMyy" dfdl:calendarPatternKind="explicit" | ||
| dfdl:lengthKind="explicit" dfdl:length="{ 16 }" dfdl:lengthUnits="bits" dfdl:binaryCalendarRep="bcd"/> | ||
| dfdl:lengthKind="explicit" dfdl:length="{ 2 }" dfdl:lengthUnits="bytes" dfdl:binaryCalendarRep="bcd"/> |
There was a problem hiding this comment.
Feels like these tests are intentionally testing that lengthUnits=bits works for bcd. Do we want to allow that (and continue to test for it), and create a bug to remove that warning.
There was a problem hiding this comment.
So we have the ticket DAFFODIL-2931, which needs to be updated to reflect removing the warning based on the convos in the devlist (https://lists.apache.org/thread/01sn3fk2wk7jtos2svw4995f7dxxt7g1). I will reset this change
| <xs:group name="hg"> | ||
| <xs:choice> | ||
| <xs:element name="inty" type="xs:int" | ||
| <xs:element name="hg_inty" type="xs:int" |
There was a problem hiding this comment.
Was this warning about siblings with the same name, so you made them unique? That might be a sign that our warning is incorrect? One of these groups is hidden group so it won't end up in the infoset and so should not trigger the same name warning.
There was a problem hiding this comment.
Yes, we were getting the following warning, though now I'm noticing the namespaces are the same. So most likely it is a bug
Schema Definition Warning: Neighboring QNames differ only by namespaces. Infoset representations that do not support namespaces cannot differentiate between these elements and may fail to unparse. QNames are: ex:{http://example.com}inty, ex:{http://example.com}inty (id: namespaceDifferencesOnly) Schema context: group[4] Location line 1185 in /org/apache/daffodil/section14/sequence_groups/SequenceGroup.tdml
There was a problem hiding this comment.
Looks like the namespaces are both the same http://example.com? Might be a couple bugs? We should be ignoring hidden groups when checking neighboring qnames, but also seems like we sometimes dont' detect that the namespaces don't actually differ?
| <!-- Must be utf-8, or it chooses the other backend which does more copying and so doesn't show up the bug. --> | ||
| <xs:include schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormatPortable.dfdl.xsd"/> | ||
| <dfdl:format ref="ex:GeneralFormat" encoding="UTF-8" lengthKind="delimited" /> | ||
| <dfdl:format ref="ex:GeneralFormat" encoding="UTF-8" lengthKind="delimited" encodingErrorPolicy="replace" /> |
There was a problem hiding this comment.
For all these changes to encodingErrorPolicy="replace" do we want to instead switch to using the non-portable DFDLGeneralFormat.dfdl.xsd? If we are breaking portability by using "replace" (which is probably fine for tests that aren't explicitly testing for it) that might make it more clear that these aren't intended to be portable.
olabusayoT
force-pushed
the
daf-2926-addressUnexpectedWarnings
branch
from
dee7ebb to
2466eaf
Compare
- fix schemas to remove some unexpected warnings - remove encodingErrorPolicy replace and use GeneralFormat instead - add suppressSchemaDefinitionWarnings to some schema elements DAFFODIL-2926
olabusayoT
force-pushed
the
daf-2926-addressUnexpectedWarnings
branch
from
2466eaf to
ef0fab5
Compare
3 participants
NOTE: this is not a complete fix as some of the warnings are bugs needing fixing, or require use of daf:suppressSchemaDefinitionWarnings on root elements etc
DAFFODIL-2926