Added dfdlx:alignmentViaSkip property#770
Conversation
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+1, only thought about the name of the property, but I don't feel strongly about it. It's experimental so we can always change the name later if needed
tuxji
left a comment
tuxji
left a comment
There was a problem hiding this comment.
+1 once comments are addressed
tuxji
left a comment
tuxji
left a comment
There was a problem hiding this comment.
+1 with comments about comments :)
| prop match { | ||
| case NotFound(_, _, _) => AlignmentKind.Automatic // default since this is a new property | ||
| case Found("automatic",_,_,_) => AlignmentKind.Automatic | ||
| case Found("manual",_,_,_) => AlignmentKind.Manual | ||
| case _ => Assert.impossible("Per validation, can only be 'automatic' or 'manual'") | ||
| } |
There was a problem hiding this comment.
Much more readable now! Even covers the impossible case so readers will understand what's happening.
There was a problem hiding this comment.
Agreed.
A second thought, why are we even manually implementing this? Shouldn't the property generator be able to easily generate code for this? Or does that not handle extension properties or something?
I guess we'd still need something manual to provide a default, but this could look something like
lazy val alignmentKindDefaulted = optionAlignmentKind.getOrElse(AlignmentKind.Automatic)I think we use a similar pattern for other properties that have a default value?
There was a problem hiding this comment.
You know, that may be the case. I may have over complicated this. This is a regular property, except extension namespace, and default value.
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+1 to changes, minor comments
| prop match { | ||
| case NotFound(_, _, _) => AlignmentKind.Automatic // default since this is a new property | ||
| case Found("automatic",_,_,_) => AlignmentKind.Automatic | ||
| case Found("manual",_,_,_) => AlignmentKind.Manual | ||
| case _ => Assert.impossible("Per validation, can only be 'automatic' or 'manual'") | ||
| } |
There was a problem hiding this comment.
Agreed.
A second thought, why are we even manually implementing this? Shouldn't the property generator be able to easily generate code for this? Or does that not handle extension properties or something?
I guess we'd still need something manual to provide a default, but this could look something like
lazy val alignmentKindDefaulted = optionAlignmentKind.getOrElse(AlignmentKind.Automatic)I think we use a similar pattern for other properties that have a default value?
mbeckerle
force-pushed
the
daf-2652-alignprop
branch
2 times, most recently
from
3ec0055 to
fd2e6b5
Compare
ParseError on charset not aligned. This is a significant change in behavior, though because the compiler puts down mandatory text alignment fill regions the automatic alignmentby the I/O layer was redundant. Without the mandatory text alignment regions, the I/O layer really must not do auto alignment. Removed dead code from TextParser and unused I/O layer getString method. DAFFODIL-2652
mbeckerle
force-pushed
the
daf-2652-alignprop
branch
from
fd2e6b5 to
79d06ce
Compare
3 participants
DAFFODIL-2652