mbeckerle commented on a change in pull request #117: Allow DFDL expressions in the message attribute of asserts/discrimina… URL: https://github.com/apache/incubator-daffodil/pull/117#discussion_r213097712
########## File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/AssertPatternParsers.scala ########## @@ -25,17 +25,41 @@ import org.apache.daffodil.processors._ import org.apache.daffodil.util.LogLevel import org.apache.daffodil.util.OnStack Review comment: This is great. I think the refactor below is totally worth it. It's only a few lines of code, being factored out, but there are other reasons why it's worth it. It makes it clear that the behavior is intended to be exactly the same between the two uses, whereas otherwise you need to have comments pointing to/from the other piece of code saying "keep these consistent", and I hate that because my IDE will jump to getAssertFailureMessage in the mixin for me, but the comments I have to navigate around myself with grep/search, and the two blocks of code can drift, and if by accident somebody fixes one and not the other, then I have to wonder if the two were intended to diverge, and the comments not fixed, or they were intended to be the same, but just not updated simultaneously. In general, I hate doing in comments what I can do by way of code constructs. A trait is a pretty easy way to share even just a single small method. I also think the method name getAssertFailureMessage() adds value to the code. Makes it more readable. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
