You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2020/09/11 21:48:36 UTC
[GitHub] [incubator-daffodil] IanCarlsonOwl opened a new pull request #418: Add warnings for properties which look like expressions
IanCarlsonOwl opened a new pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418
First attempt at a pull request against the Daffodil repo - please examine carefully.
DAFFODIL-879
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r493818932
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section00/general/testExpressionPropertyWarnings.tdml
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ suiteName="test of warnings for when a property can't be an expression but looks like an expression"
+ defaultRoundTrip="onePass">
+
+ <tdml:defineSchema name="S1" elementFormDefault="unqualified">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited"/>
+ <xs:element name="root" type="xs:double" dfdl:textNumberPattern="#####0.0#####" dfdl:textStandardInfinityRep="{../ex:thisCantBeAnExpression}" />
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="expressionPropertyWarning1" model="S1" root="root">
+ <tdml:document>1.0</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:root xsi:type="xs:double">1.0</ex:root>
Review comment:
I thought the comparison did type specific equality using xsi:type information?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489671084
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +50,71 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", forExpression)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", forExpression)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", forExpression)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected def forExpression: Boolean
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", forExpression)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", forExpression)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", forExpression)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", forExpression)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
+ final protected def forExpression = true
Review comment:
I'm not opposed - calling out the parameter by name does seem like it'll make the purpose more transparent. I'll make that change if there's no disagreement from others.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r488210588
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
I'm definitely in favor of enabling scalariform. [DAFFODIL-2133](https://issues.apache.org/jira/browse/DAFFODIL-2133) is the issue for that. I'd like to enable the check on pull requests so it fails an PR's that don't follow the Daffodil style guidelines. The problem is I'm sure a lot of our code doesn't currently match the style guides, so we'd need to fix that first. Probably not a small task.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489575216
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -21,25 +21,26 @@ import org.apache.daffodil.schema.annotation.props.PropertyMixin
trait RawCommonRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder")
+ final protected def forExpression = true
Review comment:
My understanding of this exchange is that there's no change to be made at the moment. Is that correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489727250
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +50,71 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", forExpression)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", forExpression)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", forExpression)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected def forExpression: Boolean
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", forExpression)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", forExpression)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", forExpression)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", forExpression)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
+ final protected def forExpression = true
Review comment:
I suggest the expressionAllowed name is better than forExpression.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r493754890
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +144,21 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ // if this looks like it could be an expression, issue a warning
+ // but only if expressionAllowed is false
+ if(!expressionAllowed) {
+ propRes match {
+ case Found(v, _, _, _) => {
+ if(DPathUtil.isExpression(v)) {
Review comment:
Made a fix-up commit before rebasing that resolves this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#issuecomment-697991383
I have squashed all commits in to one. I believe this pull request is ready to close, along with DAFFODIL-879
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489573427
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
Moved DPathUtil from daffodil-runtime1 to daffodil-lib and invoked isExpression rather than forming the check here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489572009
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line wrapped before ""Property"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r488905326
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -21,25 +21,26 @@ import org.apache.daffodil.schema.annotation.props.PropertyMixin
trait RawCommonRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder")
+ final protected def forExpression = true
Review comment:
Likely not, because all the runtime-valued properties are not auto-generated by propgen, but are part of a by-hand mixin.
The generated properties are either never expressions or always expressions. The only ones we have to deal with are the ones that are sometimes expressions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r492861078
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +144,21 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ // if this looks like it could be an expression, issue a warning
+ // but only if expressionAllowed is false
+ if(!expressionAllowed) {
+ propRes match {
+ case Found(v, _, _, _) => {
+ if(DPathUtil.isExpression(v)) {
Review comment:
Formatting: should be `if (` not `if(`
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/CompiledExpression.scala
##########
@@ -23,6 +23,7 @@ import scala.xml.NamespaceBinding
import org.apache.daffodil.xml.NamedQName
import java.lang.{ Long => JLong, Boolean => JBoolean }
import org.apache.daffodil.schema.annotation.props.Found
+import org.apache.daffodil.util.DPathUtil
Review comment:
Is this import really used? Is there an `import org.apache.daffodil.dpath.DPathUtil` that should be removed?
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +144,21 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ // if this looks like it could be an expression, issue a warning
+ // but only if expressionAllowed is false
+ if(!expressionAllowed) {
Review comment:
Formatting: should be `if (!expressionAllowed) {` not `if(!expressionAllowed) {`
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section00/general/testExpressionPropertyWarnings.tdml
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ suiteName="test of warnings for when a property can't be an expression but looks like an expression"
+ defaultRoundTrip="onePass">
+
+ <tdml:defineSchema name="S1" elementFormDefault="unqualified">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited"/>
+ <xs:element name="root" type="xs:double" dfdl:textNumberPattern="#####0.0#####" dfdl:textStandardInfinityRep="{../ex:thisCantBeAnExpression}" />
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="expressionPropertyWarning1" model="S1" root="root">
+ <tdml:document>1.0</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:root xsi:type="xs:double">1.0</ex:root>
Review comment:
Why do you have an `xsi:type="xs:double"` when the schema already declares the root element with the xs:double type?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489579405
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +50,71 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", forExpression)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", forExpression)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", forExpression)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected def forExpression: Boolean
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", forExpression)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", forExpression)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", forExpression)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", forExpression)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
+ final protected def forExpression = true
Review comment:
Thoughts on instead of redefining forExpression for all these mixins, we just do something like
```scala
findPropertyOption("escapeCharacter", isforExpression = true)
```
It's not significantly longer and feels a bit more self-documenting to me.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +144,21 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ // if this looks like it could be an expression, issue a warning
+ // but only if expressionAllowed is false
+ if(!expressionAllowed) {
+ propRes match {
+ case Found(v, _, _, _) => {
+ if(DPathUtil.isExpression(v)) {
+ SDW(WarnID.NonExpressionPropertyValueLooksLikeExpression,
+ "Property %s looks like an expression but cannot be an expression: %s", pname, v)
+ }
+ }
+ case _ => {
+ // Do Nothing
+ }
+ }
+ }
Review comment:
Needs a space after the ``if`` keyword above.
Also, this is just personal style preference, but all the indentions make this kind of hard to read to me. Anyone else get that, or is it just me? I'm wondering if something like this would be preferred?
```scala
if (!expressionAllowed) {
val isExpression = propRes.toOption.map { DPathUtil.isExpression(_) }.getOrElse(false)
schemaDefinitionWarningUnless(
WarnID....,
!isExpression,
"Property ..."
)
}
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -21,25 +21,26 @@ import org.apache.daffodil.schema.annotation.props.PropertyMixin
trait RawCommonRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder")
+ final protected def forExpression = true
Review comment:
I think so. I've skimmed the autogenerated mixes and Mike is right. All those mixins cannot have expressions, and so the default value of ``isForExpression = false`` is correct for all of them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +128,12 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ /**
+ * expressionAllowed describes whether a run-time expression
+ * is valid for this option. Used to decide whether a warning
+ * should be produced if the value "looks like" an expression
+ */
+ def findPropertyOption(pname: String, expressionAllowed: Boolean = false): PropertyLookupResult = {
Review comment:
There's inconsistent argument naming. Above the argument is ``isForExpression``, but here it's ``expressionAllowed``. These arguments are essentially the saeme so should probably have the same name.
And I think Mike suggested something in a comment that maybe another name might be more self documenting? I have no stong preference on name, but we should be consistent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r493726441
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section00/general/testExpressionPropertyWarnings.tdml
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ suiteName="test of warnings for when a property can't be an expression but looks like an expression"
+ defaultRoundTrip="onePass">
+
+ <tdml:defineSchema name="S1" elementFormDefault="unqualified">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited"/>
+ <xs:element name="root" type="xs:double" dfdl:textNumberPattern="#####0.0#####" dfdl:textStandardInfinityRep="{../ex:thisCantBeAnExpression}" />
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="expressionPropertyWarning1" model="S1" root="root">
+ <tdml:document>1.0</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:root xsi:type="xs:double">1.0</ex:root>
Review comment:
Hmm, I don't think Daffodil takes into account ``xsi:type`` except for a comparing expected vs actual results. And even then, it only uses it it for ``xs:anyURI`` and calendar types (e.g. ``xs:dateTime``). I'm pretty sure ``xsi:type="xs:double"`` shouldn't have an affect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r493699779
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/CompiledExpression.scala
##########
@@ -23,6 +23,7 @@ import scala.xml.NamespaceBinding
import org.apache.daffodil.xml.NamedQName
import java.lang.{ Long => JLong, Boolean => JBoolean }
import org.apache.daffodil.schema.annotation.props.Found
+import org.apache.daffodil.util.DPathUtil
Review comment:
The DPathUtil import is used for the new call to DPathUtil.isExpression on line 64.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489726004
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +128,12 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ /**
+ * expressionAllowed describes whether a run-time expression
+ * is valid for this option. Used to decide whether a warning
+ * should be produced if the value "looks like" an expression
+ */
+ def findPropertyOption(pname: String, expressionAllowed: Boolean = false): PropertyLookupResult = {
Review comment:
Note that we DONT need this argument passed as true for things that MUST be expressions. Only for things that are optionally expressions. Hence, the name should reflect that conditionality, and scaladoc should make this clear.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r493824585
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section00/general/testExpressionPropertyWarnings.tdml
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ suiteName="test of warnings for when a property can't be an expression but looks like an expression"
+ defaultRoundTrip="onePass">
+
+ <tdml:defineSchema name="S1" elementFormDefault="unqualified">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited"/>
+ <xs:element name="root" type="xs:double" dfdl:textNumberPattern="#####0.0#####" dfdl:textStandardInfinityRep="{../ex:thisCantBeAnExpression}" />
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="expressionPropertyWarning1" model="S1" root="root">
+ <tdml:document>1.0</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:root xsi:type="xs:double">1.0</ex:root>
Review comment:
It does, but not for numeric types--only for xs:anyURI, xs:hexBinary, xs:time, and xs:dateTime. These are the only ones where IBM DFDL and Daffodil have different output so we needed a little extra smarts. We could add it for the remainder of types, but haven't had the need so far.
https://github.com/apache/incubator-daffodil/blob/master/daffodil-lib/src/main/scala/org/apache/daffodil/xml/XMLUtils.scala#L1088-L1108
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r494253691
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +49,68 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", expressionAllowed = true)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", expressionAllowed = true)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", expressionAllowed = true)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", expressionAllowed = true)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", expressionAllowed = true)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", expressionAllowed = true)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", expressionAllowed = true)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
-
// package private because used in unit test
- final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter")
+ final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter", expressionAllowed = true)
private[dsom] final lazy val escapeCharacterRaw = requireProperty(optionEscapeCharacterRaw)
- final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter")
+ final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter", expressionAllowed = true)
protected final lazy val escapeEscapeCharacterRaw = requireProperty(optionEscapeEscapeCharacterRaw)
- protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart")
+ protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart", expressionAllowed = true)
protected final lazy val escapeBlockStartRaw = requireProperty(optionEscapeBlockStartRaw)
- protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd")
+ protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd", expressionAllowed = true)
protected final lazy val escapeBlockEndRaw = requireProperty(optionEscapeBlockEndRaw)
- protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters")
+ protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters", expressionAllowed = true)
Review comment:
escapeBlockStart, escapeBlockEnd, and extraEscapedCharacters cannot be expressions:
https://daffodil.apache.org/docs/dfdl/#_Toc398030751
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -21,25 +21,25 @@ import org.apache.daffodil.schema.annotation.props.PropertyMixin
trait RawCommonRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder")
+ protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder", expressionAllowed = true)
protected final lazy val byteOrderRaw = requireProperty(optionByteOrderRaw)
- protected final lazy val optionEncodingRaw = findPropertyOption("encoding")
+ protected final lazy val optionEncodingRaw = findPropertyOption("encoding", expressionAllowed = true)
protected final lazy val encodingRaw = requireProperty(optionEncodingRaw)
- protected final lazy val optionOutputNewLineRaw = findPropertyOption("outputNewLine")
+ protected final lazy val optionOutputNewLineRaw = findPropertyOption("outputNewLine", expressionAllowed = true)
protected final lazy val outputNewLineRaw = requireProperty(optionOutputNewLineRaw)
- protected final lazy val optionFillByteRaw = findPropertyOption("fillByte")
+ protected final lazy val optionFillByteRaw = findPropertyOption("fillByte", expressionAllowed = true)
Review comment:
fillByte is a DFDL String Literal and so cannot be an expression:
https://daffodil.apache.org/docs/dfdl/#_Toc130873645
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -21,25 +21,25 @@ import org.apache.daffodil.schema.annotation.props.PropertyMixin
trait RawCommonRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder")
+ protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder", expressionAllowed = true)
protected final lazy val byteOrderRaw = requireProperty(optionByteOrderRaw)
- protected final lazy val optionEncodingRaw = findPropertyOption("encoding")
+ protected final lazy val optionEncodingRaw = findPropertyOption("encoding", expressionAllowed = true)
protected final lazy val encodingRaw = requireProperty(optionEncodingRaw)
- protected final lazy val optionOutputNewLineRaw = findPropertyOption("outputNewLine")
+ protected final lazy val optionOutputNewLineRaw = findPropertyOption("outputNewLine", expressionAllowed = true)
protected final lazy val outputNewLineRaw = requireProperty(optionOutputNewLineRaw)
- protected final lazy val optionFillByteRaw = findPropertyOption("fillByte")
+ protected final lazy val optionFillByteRaw = findPropertyOption("fillByte", expressionAllowed = true)
protected final lazy val fillByteRaw = requireProperty(optionFillByteRaw)
}
trait RawDelimitedRuntimeValuedPropertiesMixin
extends RawCommonRuntimeValuedPropertiesMixin {
- protected final lazy val optionInitiatorRaw = findPropertyOption("initiator")
+ protected final lazy val optionInitiatorRaw = findPropertyOption("initiator", expressionAllowed = true)
protected final lazy val initiatorRaw = requireProperty(optionInitiatorRaw)
- protected final lazy val optionTerminatorRaw = findPropertyOption("terminator")
+ protected final lazy val optionTerminatorRaw = findPropertyOption("terminator", expressionAllowed = true)
protected final lazy val terminatorRaw = requireProperty(optionTerminatorRaw)
- protected final lazy val optionChoiceLengthRaw = findPropertyOption("choiceLength")
+ protected final lazy val optionChoiceLengthRaw = findPropertyOption("choiceLength", expressionAllowed = true)
Review comment:
choiceLength also does not allow expressions:
https://daffodil.apache.org/docs/dfdl/#_Toc398030783
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +49,68 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", expressionAllowed = true)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", expressionAllowed = true)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", expressionAllowed = true)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", expressionAllowed = true)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", expressionAllowed = true)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", expressionAllowed = true)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", expressionAllowed = true)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
-
// package private because used in unit test
- final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter")
+ final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter", expressionAllowed = true)
private[dsom] final lazy val escapeCharacterRaw = requireProperty(optionEscapeCharacterRaw)
- final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter")
+ final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter", expressionAllowed = true)
protected final lazy val escapeEscapeCharacterRaw = requireProperty(optionEscapeEscapeCharacterRaw)
- protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart")
+ protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart", expressionAllowed = true)
protected final lazy val escapeBlockStartRaw = requireProperty(optionEscapeBlockStartRaw)
- protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd")
+ protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd", expressionAllowed = true)
protected final lazy val escapeBlockEndRaw = requireProperty(optionEscapeBlockEndRaw)
- protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters")
+ protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters", expressionAllowed = true)
protected final lazy val extraEscapedCharactersRaw = requireProperty(optionExtraEscapedCharactersRaw)
}
trait RawSimpleTypeRuntimeValuedPropertiesMixin
extends RawCommonRuntimeValuedPropertiesMixin {
- protected final lazy val optionTextStandardDecimalSeparatorRaw = findPropertyOption("textStandardDecimalSeparator")
+ protected final lazy val optionTextStandardDecimalSeparatorRaw = findPropertyOption("textStandardDecimalSeparator", expressionAllowed = true)
protected final lazy val textStandardDecimalSeparatorRaw = requireProperty(optionTextStandardDecimalSeparatorRaw)
- protected final lazy val optionTextStandardGroupingSeparatorRaw = findPropertyOption("textStandardGroupingSeparator")
+ protected final lazy val optionTextStandardGroupingSeparatorRaw = findPropertyOption("textStandardGroupingSeparator", expressionAllowed = true)
protected final lazy val textStandardGroupingSeparatorRaw = requireProperty(optionTextStandardGroupingSeparatorRaw)
- protected final lazy val optionBinaryFloatRepRaw = findPropertyOption("binaryFloatRep")
+ protected final lazy val optionBinaryFloatRepRaw = findPropertyOption("binaryFloatRep", expressionAllowed = true)
protected final lazy val binaryFloatRepRaw = requireProperty(optionBinaryFloatRepRaw)
- protected final lazy val optionTextBooleanTrueRepRaw = findPropertyOption("textBooleanTrueRep")
+ protected final lazy val optionTextBooleanTrueRepRaw = findPropertyOption("textBooleanTrueRep", expressionAllowed = true)
protected final lazy val textBooleanTrueRepRaw = requireProperty(optionTextBooleanTrueRepRaw)
- protected final lazy val optionTextBooleanFalseRepRaw = findPropertyOption("textBooleanFalseRep")
+ protected final lazy val optionTextBooleanFalseRepRaw = findPropertyOption("textBooleanFalseRep", expressionAllowed = true)
protected final lazy val textBooleanFalseRepRaw = requireProperty(optionTextBooleanFalseRepRaw)
- protected final lazy val optionCalendarLanguageRaw = findPropertyOption("calendarLanguage")
+ protected final lazy val optionCalendarLanguageRaw = findPropertyOption("calendarLanguage", expressionAllowed = true)
protected final lazy val calendarLanguageRaw = requireProperty(optionCalendarLanguageRaw)
- protected final lazy val optionBinaryBooleanTrueRepRaw = findPropertyOption("binaryBooleanTrueRep")
+ protected final lazy val optionBinaryBooleanTrueRepRaw = findPropertyOption("binaryBooleanTrueRep", expressionAllowed = true)
protected final lazy val binaryBooleanTrueRepRaw = requireProperty(optionBinaryBooleanTrueRepRaw)
- protected final lazy val optionBinaryBooleanFalseRepRaw = findPropertyOption("binaryBooleanFalseRep")
+ protected final lazy val optionBinaryBooleanFalseRepRaw = findPropertyOption("binaryBooleanFalseRep", expressionAllowed = true)
Review comment:
binaryBooleanTrueRep and binaryBooleanFalseRep cannot be an expression:
https://daffodil.apache.org/docs/dfdl/#_Toc398030762
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r487308164
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r487312467
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r492861078
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +144,21 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ // if this looks like it could be an expression, issue a warning
+ // but only if expressionAllowed is false
+ if(!expressionAllowed) {
+ propRes match {
+ case Found(v, _, _, _) => {
+ if(DPathUtil.isExpression(v)) {
Review comment:
Formatting: should be `if (` not `if(`
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/CompiledExpression.scala
##########
@@ -23,6 +23,7 @@ import scala.xml.NamespaceBinding
import org.apache.daffodil.xml.NamedQName
import java.lang.{ Long => JLong, Boolean => JBoolean }
import org.apache.daffodil.schema.annotation.props.Found
+import org.apache.daffodil.util.DPathUtil
Review comment:
Is this import really used? Is there an `import org.apache.daffodil.dpath.DPathUtil` that should be removed?
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +144,21 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ // if this looks like it could be an expression, issue a warning
+ // but only if expressionAllowed is false
+ if(!expressionAllowed) {
Review comment:
Formatting: should be `if (!expressionAllowed) {` not `if(!expressionAllowed) {`
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section00/general/testExpressionPropertyWarnings.tdml
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ suiteName="test of warnings for when a property can't be an expression but looks like an expression"
+ defaultRoundTrip="onePass">
+
+ <tdml:defineSchema name="S1" elementFormDefault="unqualified">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited"/>
+ <xs:element name="root" type="xs:double" dfdl:textNumberPattern="#####0.0#####" dfdl:textStandardInfinityRep="{../ex:thisCantBeAnExpression}" />
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="expressionPropertyWarning1" model="S1" root="root">
+ <tdml:document>1.0</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:root xsi:type="xs:double">1.0</ex:root>
Review comment:
Why do you have an `xsi:type="xs:double"` when the schema already declares the root element with the xs:double type?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r488871448
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
Confirmed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r493867920
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section00/general/testExpressionPropertyWarnings.tdml
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ suiteName="test of warnings for when a property can't be an expression but looks like an expression"
+ defaultRoundTrip="onePass">
+
+ <tdml:defineSchema name="S1" elementFormDefault="unqualified">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited"/>
+ <xs:element name="root" type="xs:double" dfdl:textNumberPattern="#####0.0#####" dfdl:textStandardInfinityRep="{../ex:thisCantBeAnExpression}" />
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="expressionPropertyWarning1" model="S1" root="root">
+ <tdml:document>1.0</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:root xsi:type="xs:double">1.0</ex:root>
Review comment:
Ah, so for now, the only way to get a test like this to work is to make sure textNumberPattern constraints the number layout in the data, and use true values in the infoset, or just use strings as the element types.
Honestly, I think we do need this for double and float, as they are so snarky about comparison. Without type/schema, it's all strings, so, 1 and 1.0 in XML are not equivalent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489728488
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Looks good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] tuxji commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
tuxji commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r488177066
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Agree comment should use //, also note comment misspells expression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
That line (as well as a couple other lines) also has spaces between the parentheses which should be removed to fit Daffodil's style guide. Just for curiosity I googled scalariform (a formatter I remember hearing about) and I found this page which shows how to set up sbt to format the code while building it or use a pre-commit hook to ensure the code remains formatted consistently. Should we (the Daffodil committers) consider doing that for Daffodil?
<https://leaks.wanari.com/2017/05/04/scala-code-formatters>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489689576
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +128,12 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ /**
+ * expressionAllowed describes whether a run-time expression
+ * is valid for this option. Used to decide whether a warning
+ * should be produced if the value "looks like" an expression
+ */
+ def findPropertyOption(pname: String, expressionAllowed: Boolean = false): PropertyLookupResult = {
Review comment:
Updated both instances to use the same expressionAllowed name. My only concern with the naming convention is that both isForExpression and expressionAllowed are a bit misleading in the case of fields which must be expressions. However, expressionAllowedButNotRequired is a bit unwieldy.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r488222721
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Actually, enabling sclariform probably isn't a ton of work since it autoformats everything. I guess the real work is figuring out if any of the defaults need to be changed (hopefully not, hopefully we following the Scala style guides pretty closely), and then reviewing all of the changes. And hopefully a bunch of formatting changes wouldn't be too difficult to review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r488876586
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -21,25 +21,26 @@ import org.apache.daffodil.schema.annotation.props.PropertyMixin
trait RawCommonRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder")
+ final protected def forExpression = true
Review comment:
It looks like we only ever modify that mixins that we do by hand. There are also a bunch of mixins (most of the mixes) that are autogenerated by running "sbt genManaged". Do we need to update the daffodil-propgen subproject to add these forExpression things as well where appropriate?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r488875739
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
DPathUtil has an ``isExpression`` function which I think has that exact logic. Probably makes sense to just reuse that so if what an expression looks like ever changes we only have to update that one function.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r494581316
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +49,68 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", expressionAllowed = true)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", expressionAllowed = true)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", expressionAllowed = true)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", expressionAllowed = true)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", expressionAllowed = true)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", expressionAllowed = true)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", expressionAllowed = true)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
-
// package private because used in unit test
- final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter")
+ final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter", expressionAllowed = true)
private[dsom] final lazy val escapeCharacterRaw = requireProperty(optionEscapeCharacterRaw)
- final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter")
+ final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter", expressionAllowed = true)
protected final lazy val escapeEscapeCharacterRaw = requireProperty(optionEscapeEscapeCharacterRaw)
- protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart")
+ protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart", expressionAllowed = true)
protected final lazy val escapeBlockStartRaw = requireProperty(optionEscapeBlockStartRaw)
- protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd")
+ protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd", expressionAllowed = true)
protected final lazy val escapeBlockEndRaw = requireProperty(optionEscapeBlockEndRaw)
- protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters")
+ protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters", expressionAllowed = true)
protected final lazy val extraEscapedCharactersRaw = requireProperty(optionExtraEscapedCharactersRaw)
}
trait RawSimpleTypeRuntimeValuedPropertiesMixin
extends RawCommonRuntimeValuedPropertiesMixin {
- protected final lazy val optionTextStandardDecimalSeparatorRaw = findPropertyOption("textStandardDecimalSeparator")
+ protected final lazy val optionTextStandardDecimalSeparatorRaw = findPropertyOption("textStandardDecimalSeparator", expressionAllowed = true)
protected final lazy val textStandardDecimalSeparatorRaw = requireProperty(optionTextStandardDecimalSeparatorRaw)
- protected final lazy val optionTextStandardGroupingSeparatorRaw = findPropertyOption("textStandardGroupingSeparator")
+ protected final lazy val optionTextStandardGroupingSeparatorRaw = findPropertyOption("textStandardGroupingSeparator", expressionAllowed = true)
protected final lazy val textStandardGroupingSeparatorRaw = requireProperty(optionTextStandardGroupingSeparatorRaw)
- protected final lazy val optionBinaryFloatRepRaw = findPropertyOption("binaryFloatRep")
+ protected final lazy val optionBinaryFloatRepRaw = findPropertyOption("binaryFloatRep", expressionAllowed = true)
protected final lazy val binaryFloatRepRaw = requireProperty(optionBinaryFloatRepRaw)
- protected final lazy val optionTextBooleanTrueRepRaw = findPropertyOption("textBooleanTrueRep")
+ protected final lazy val optionTextBooleanTrueRepRaw = findPropertyOption("textBooleanTrueRep", expressionAllowed = true)
protected final lazy val textBooleanTrueRepRaw = requireProperty(optionTextBooleanTrueRepRaw)
- protected final lazy val optionTextBooleanFalseRepRaw = findPropertyOption("textBooleanFalseRep")
+ protected final lazy val optionTextBooleanFalseRepRaw = findPropertyOption("textBooleanFalseRep", expressionAllowed = true)
protected final lazy val textBooleanFalseRepRaw = requireProperty(optionTextBooleanFalseRepRaw)
- protected final lazy val optionCalendarLanguageRaw = findPropertyOption("calendarLanguage")
+ protected final lazy val optionCalendarLanguageRaw = findPropertyOption("calendarLanguage", expressionAllowed = true)
protected final lazy val calendarLanguageRaw = requireProperty(optionCalendarLanguageRaw)
- protected final lazy val optionBinaryBooleanTrueRepRaw = findPropertyOption("binaryBooleanTrueRep")
+ protected final lazy val optionBinaryBooleanTrueRepRaw = findPropertyOption("binaryBooleanTrueRep", expressionAllowed = true)
protected final lazy val binaryBooleanTrueRepRaw = requireProperty(optionBinaryBooleanTrueRepRaw)
- protected final lazy val optionBinaryBooleanFalseRepRaw = findPropertyOption("binaryBooleanFalseRep")
+ protected final lazy val optionBinaryBooleanFalseRepRaw = findPropertyOption("binaryBooleanFalseRep", expressionAllowed = true)
Review comment:
I've updated the commit to remove the allowedExpression=true from the lines where it was incorrect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r493719260
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section00/general/testExpressionPropertyWarnings.tdml
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ suiteName="test of warnings for when a property can't be an expression but looks like an expression"
+ defaultRoundTrip="onePass">
+
+ <tdml:defineSchema name="S1" elementFormDefault="unqualified">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited"/>
+ <xs:element name="root" type="xs:double" dfdl:textNumberPattern="#####0.0#####" dfdl:textStandardInfinityRep="{../ex:thisCantBeAnExpression}" />
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="expressionPropertyWarning1" model="S1" root="root">
+ <tdml:document>1.0</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:root xsi:type="xs:double">1.0</ex:root>
Review comment:
Without that decoration, the 1.0 was reduced to 1, and the round-trip parse-unparse ended up having different results on either end.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489573020
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Added a scaladoc. The autogenerated block that pops up in IntelliJ for the function header didn't seem to match other functions in the file. Does this scaladoc look ook?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489697444
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +128,12 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ /**
+ * expressionAllowed describes whether a run-time expression
+ * is valid for this option. Used to decide whether a warning
+ * should be produced if the value "looks like" an expression
+ */
+ def findPropertyOption(pname: String, expressionAllowed: Boolean = false): PropertyLookupResult = {
Review comment:
Another option might be ``canBeAnExpression``, which is a little less unwieldy and I think still implies it's not required.
But I don't feel strongly about the name, and I don't think ``expressionAllowed`` is terrible. I can't seem to find Mike's sugestion, maybe I made that up. But in general I usually prefer names thats a little unweildy but accurate over names that are shorter but maybe less accurate. We have plenty of variables that are basically sentences purely to make the code easier to understand and to be more meaningful.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489574291
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
I've updated the formatting to be more in line with the rest of the file. I don't see any additional changes that need to be made in this context given the existence of DAFFODIL-2133
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489572529
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Replaced /* */ with //
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489571740
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
Blank line removed, and space between ! and isForExpression removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r487308164
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r494253691
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +49,68 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", expressionAllowed = true)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", expressionAllowed = true)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", expressionAllowed = true)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", expressionAllowed = true)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", expressionAllowed = true)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", expressionAllowed = true)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", expressionAllowed = true)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
-
// package private because used in unit test
- final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter")
+ final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter", expressionAllowed = true)
private[dsom] final lazy val escapeCharacterRaw = requireProperty(optionEscapeCharacterRaw)
- final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter")
+ final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter", expressionAllowed = true)
protected final lazy val escapeEscapeCharacterRaw = requireProperty(optionEscapeEscapeCharacterRaw)
- protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart")
+ protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart", expressionAllowed = true)
protected final lazy val escapeBlockStartRaw = requireProperty(optionEscapeBlockStartRaw)
- protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd")
+ protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd", expressionAllowed = true)
protected final lazy val escapeBlockEndRaw = requireProperty(optionEscapeBlockEndRaw)
- protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters")
+ protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters", expressionAllowed = true)
Review comment:
escapeBlockStart, escapeBlockEnd, and extraEscapedCharacters cannot be expressions:
https://daffodil.apache.org/docs/dfdl/#_Toc398030751
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -21,25 +21,25 @@ import org.apache.daffodil.schema.annotation.props.PropertyMixin
trait RawCommonRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder")
+ protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder", expressionAllowed = true)
protected final lazy val byteOrderRaw = requireProperty(optionByteOrderRaw)
- protected final lazy val optionEncodingRaw = findPropertyOption("encoding")
+ protected final lazy val optionEncodingRaw = findPropertyOption("encoding", expressionAllowed = true)
protected final lazy val encodingRaw = requireProperty(optionEncodingRaw)
- protected final lazy val optionOutputNewLineRaw = findPropertyOption("outputNewLine")
+ protected final lazy val optionOutputNewLineRaw = findPropertyOption("outputNewLine", expressionAllowed = true)
protected final lazy val outputNewLineRaw = requireProperty(optionOutputNewLineRaw)
- protected final lazy val optionFillByteRaw = findPropertyOption("fillByte")
+ protected final lazy val optionFillByteRaw = findPropertyOption("fillByte", expressionAllowed = true)
Review comment:
fillByte is a DFDL String Literal and so cannot be an expression:
https://daffodil.apache.org/docs/dfdl/#_Toc130873645
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -21,25 +21,25 @@ import org.apache.daffodil.schema.annotation.props.PropertyMixin
trait RawCommonRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder")
+ protected final lazy val optionByteOrderRaw = findPropertyOption("byteOrder", expressionAllowed = true)
protected final lazy val byteOrderRaw = requireProperty(optionByteOrderRaw)
- protected final lazy val optionEncodingRaw = findPropertyOption("encoding")
+ protected final lazy val optionEncodingRaw = findPropertyOption("encoding", expressionAllowed = true)
protected final lazy val encodingRaw = requireProperty(optionEncodingRaw)
- protected final lazy val optionOutputNewLineRaw = findPropertyOption("outputNewLine")
+ protected final lazy val optionOutputNewLineRaw = findPropertyOption("outputNewLine", expressionAllowed = true)
protected final lazy val outputNewLineRaw = requireProperty(optionOutputNewLineRaw)
- protected final lazy val optionFillByteRaw = findPropertyOption("fillByte")
+ protected final lazy val optionFillByteRaw = findPropertyOption("fillByte", expressionAllowed = true)
protected final lazy val fillByteRaw = requireProperty(optionFillByteRaw)
}
trait RawDelimitedRuntimeValuedPropertiesMixin
extends RawCommonRuntimeValuedPropertiesMixin {
- protected final lazy val optionInitiatorRaw = findPropertyOption("initiator")
+ protected final lazy val optionInitiatorRaw = findPropertyOption("initiator", expressionAllowed = true)
protected final lazy val initiatorRaw = requireProperty(optionInitiatorRaw)
- protected final lazy val optionTerminatorRaw = findPropertyOption("terminator")
+ protected final lazy val optionTerminatorRaw = findPropertyOption("terminator", expressionAllowed = true)
protected final lazy val terminatorRaw = requireProperty(optionTerminatorRaw)
- protected final lazy val optionChoiceLengthRaw = findPropertyOption("choiceLength")
+ protected final lazy val optionChoiceLengthRaw = findPropertyOption("choiceLength", expressionAllowed = true)
Review comment:
choiceLength also does not allow expressions:
https://daffodil.apache.org/docs/dfdl/#_Toc398030783
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +49,68 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", expressionAllowed = true)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", expressionAllowed = true)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", expressionAllowed = true)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", expressionAllowed = true)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", expressionAllowed = true)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", expressionAllowed = true)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", expressionAllowed = true)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
-
// package private because used in unit test
- final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter")
+ final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter", expressionAllowed = true)
private[dsom] final lazy val escapeCharacterRaw = requireProperty(optionEscapeCharacterRaw)
- final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter")
+ final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter", expressionAllowed = true)
protected final lazy val escapeEscapeCharacterRaw = requireProperty(optionEscapeEscapeCharacterRaw)
- protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart")
+ protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart", expressionAllowed = true)
protected final lazy val escapeBlockStartRaw = requireProperty(optionEscapeBlockStartRaw)
- protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd")
+ protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd", expressionAllowed = true)
protected final lazy val escapeBlockEndRaw = requireProperty(optionEscapeBlockEndRaw)
- protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters")
+ protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters", expressionAllowed = true)
protected final lazy val extraEscapedCharactersRaw = requireProperty(optionExtraEscapedCharactersRaw)
}
trait RawSimpleTypeRuntimeValuedPropertiesMixin
extends RawCommonRuntimeValuedPropertiesMixin {
- protected final lazy val optionTextStandardDecimalSeparatorRaw = findPropertyOption("textStandardDecimalSeparator")
+ protected final lazy val optionTextStandardDecimalSeparatorRaw = findPropertyOption("textStandardDecimalSeparator", expressionAllowed = true)
protected final lazy val textStandardDecimalSeparatorRaw = requireProperty(optionTextStandardDecimalSeparatorRaw)
- protected final lazy val optionTextStandardGroupingSeparatorRaw = findPropertyOption("textStandardGroupingSeparator")
+ protected final lazy val optionTextStandardGroupingSeparatorRaw = findPropertyOption("textStandardGroupingSeparator", expressionAllowed = true)
protected final lazy val textStandardGroupingSeparatorRaw = requireProperty(optionTextStandardGroupingSeparatorRaw)
- protected final lazy val optionBinaryFloatRepRaw = findPropertyOption("binaryFloatRep")
+ protected final lazy val optionBinaryFloatRepRaw = findPropertyOption("binaryFloatRep", expressionAllowed = true)
protected final lazy val binaryFloatRepRaw = requireProperty(optionBinaryFloatRepRaw)
- protected final lazy val optionTextBooleanTrueRepRaw = findPropertyOption("textBooleanTrueRep")
+ protected final lazy val optionTextBooleanTrueRepRaw = findPropertyOption("textBooleanTrueRep", expressionAllowed = true)
protected final lazy val textBooleanTrueRepRaw = requireProperty(optionTextBooleanTrueRepRaw)
- protected final lazy val optionTextBooleanFalseRepRaw = findPropertyOption("textBooleanFalseRep")
+ protected final lazy val optionTextBooleanFalseRepRaw = findPropertyOption("textBooleanFalseRep", expressionAllowed = true)
protected final lazy val textBooleanFalseRepRaw = requireProperty(optionTextBooleanFalseRepRaw)
- protected final lazy val optionCalendarLanguageRaw = findPropertyOption("calendarLanguage")
+ protected final lazy val optionCalendarLanguageRaw = findPropertyOption("calendarLanguage", expressionAllowed = true)
protected final lazy val calendarLanguageRaw = requireProperty(optionCalendarLanguageRaw)
- protected final lazy val optionBinaryBooleanTrueRepRaw = findPropertyOption("binaryBooleanTrueRep")
+ protected final lazy val optionBinaryBooleanTrueRepRaw = findPropertyOption("binaryBooleanTrueRep", expressionAllowed = true)
protected final lazy val binaryBooleanTrueRepRaw = requireProperty(optionBinaryBooleanTrueRepRaw)
- protected final lazy val optionBinaryBooleanFalseRepRaw = findPropertyOption("binaryBooleanFalseRep")
+ protected final lazy val optionBinaryBooleanFalseRepRaw = findPropertyOption("binaryBooleanFalseRep", expressionAllowed = true)
Review comment:
binaryBooleanTrueRep and binaryBooleanFalseRep cannot be an expression:
https://daffodil.apache.org/docs/dfdl/#_Toc398030762
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489729351
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +50,71 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", forExpression)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", forExpression)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", forExpression)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected def forExpression: Boolean
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", forExpression)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", forExpression)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", forExpression)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", forExpression)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
+ final protected def forExpression = true
Review comment:
I like SL's suggestion that we pass this using the name.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489729677
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +128,12 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ /**
+ * expressionAllowed describes whether a run-time expression
+ * is valid for this option. Used to decide whether a warning
+ * should be produced if the value "looks like" an expression
+ */
+ def findPropertyOption(pname: String, expressionAllowed: Boolean = false): PropertyLookupResult = {
Review comment:
expressionAllowed works for me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r488868994
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
To restate - we want to warn only if the value begins and ends with { }, and doesn't begin with {{. Is that your intent?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence merged pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r494581316
##########
File path: daffodil-core/src/main/scala/org/apache/daffodil/dsom/RawCommonRuntimeValuedPropertiesMixin.scala
##########
@@ -49,69 +49,68 @@ trait RawElementRuntimeValuedPropertiesMixin
// these are almost certainly not in scope, but on the local object
// but not always. A type might define fixed length things for example.
- protected final lazy val optionLengthRaw = findPropertyOption("length")
+ protected final lazy val optionLengthRaw = findPropertyOption("length", expressionAllowed = true)
protected final lazy val lengthRaw = requireProperty(optionLengthRaw)
- protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount")
+ protected final lazy val optionOccursCountRaw = findPropertyOption("occursCount", expressionAllowed = true)
protected final lazy val occursCountRaw = requireProperty(optionOccursCountRaw)
}
trait RawSequenceRuntimeValuedPropertiesMixin
extends RawDelimitedRuntimeValuedPropertiesMixin {
- protected final lazy val optionSeparatorRaw = findPropertyOption("separator")
+ protected final lazy val optionSeparatorRaw = findPropertyOption("separator", expressionAllowed = true)
protected final lazy val separatorRaw = requireProperty(optionSeparatorRaw)
}
trait RawLayeringRuntimeValuedPropertiesMixin
extends PropertyMixin {
- protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform")
+ protected final lazy val optionLayerTransformRaw = findPropertyOption("layerTransform", expressionAllowed = true)
protected final lazy val layerTransformRaw = requireProperty(optionLayerTransformRaw)
- protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding")
+ protected final lazy val optionLayerEncodingRaw = findPropertyOption("layerEncoding", expressionAllowed = true)
protected final lazy val layerEncodingRaw = requireProperty(optionLayerEncodingRaw)
- protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength")
+ protected final lazy val optionLayerLengthRaw = findPropertyOption("layerLength", expressionAllowed = true)
protected final lazy val layerLengthRaw = requireProperty(optionLayerLengthRaw)
- protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark")
+ protected final lazy val optionLayerBoundaryMarkRaw = findPropertyOption("layerBoundaryMark", expressionAllowed = true)
protected final lazy val layerBoundaryMarkRaw = requireProperty(optionLayerBoundaryMarkRaw)
}
trait RawEscapeSchemeRuntimeValuedPropertiesMixin
extends PropertyMixin {
-
// package private because used in unit test
- final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter")
+ final lazy val optionEscapeCharacterRaw = findPropertyOption("escapeCharacter", expressionAllowed = true)
private[dsom] final lazy val escapeCharacterRaw = requireProperty(optionEscapeCharacterRaw)
- final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter")
+ final lazy val optionEscapeEscapeCharacterRaw = findPropertyOption("escapeEscapeCharacter", expressionAllowed = true)
protected final lazy val escapeEscapeCharacterRaw = requireProperty(optionEscapeEscapeCharacterRaw)
- protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart")
+ protected final lazy val optionEscapeBlockStartRaw = findPropertyOption("escapeBlockStart", expressionAllowed = true)
protected final lazy val escapeBlockStartRaw = requireProperty(optionEscapeBlockStartRaw)
- protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd")
+ protected final lazy val optionEscapeBlockEndRaw = findPropertyOption("escapeBlockEnd", expressionAllowed = true)
protected final lazy val escapeBlockEndRaw = requireProperty(optionEscapeBlockEndRaw)
- protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters")
+ protected final lazy val optionExtraEscapedCharactersRaw = findPropertyOption("extraEscapedCharacters", expressionAllowed = true)
protected final lazy val extraEscapedCharactersRaw = requireProperty(optionExtraEscapedCharactersRaw)
}
trait RawSimpleTypeRuntimeValuedPropertiesMixin
extends RawCommonRuntimeValuedPropertiesMixin {
- protected final lazy val optionTextStandardDecimalSeparatorRaw = findPropertyOption("textStandardDecimalSeparator")
+ protected final lazy val optionTextStandardDecimalSeparatorRaw = findPropertyOption("textStandardDecimalSeparator", expressionAllowed = true)
protected final lazy val textStandardDecimalSeparatorRaw = requireProperty(optionTextStandardDecimalSeparatorRaw)
- protected final lazy val optionTextStandardGroupingSeparatorRaw = findPropertyOption("textStandardGroupingSeparator")
+ protected final lazy val optionTextStandardGroupingSeparatorRaw = findPropertyOption("textStandardGroupingSeparator", expressionAllowed = true)
protected final lazy val textStandardGroupingSeparatorRaw = requireProperty(optionTextStandardGroupingSeparatorRaw)
- protected final lazy val optionBinaryFloatRepRaw = findPropertyOption("binaryFloatRep")
+ protected final lazy val optionBinaryFloatRepRaw = findPropertyOption("binaryFloatRep", expressionAllowed = true)
protected final lazy val binaryFloatRepRaw = requireProperty(optionBinaryFloatRepRaw)
- protected final lazy val optionTextBooleanTrueRepRaw = findPropertyOption("textBooleanTrueRep")
+ protected final lazy val optionTextBooleanTrueRepRaw = findPropertyOption("textBooleanTrueRep", expressionAllowed = true)
protected final lazy val textBooleanTrueRepRaw = requireProperty(optionTextBooleanTrueRepRaw)
- protected final lazy val optionTextBooleanFalseRepRaw = findPropertyOption("textBooleanFalseRep")
+ protected final lazy val optionTextBooleanFalseRepRaw = findPropertyOption("textBooleanFalseRep", expressionAllowed = true)
protected final lazy val textBooleanFalseRepRaw = requireProperty(optionTextBooleanFalseRepRaw)
- protected final lazy val optionCalendarLanguageRaw = findPropertyOption("calendarLanguage")
+ protected final lazy val optionCalendarLanguageRaw = findPropertyOption("calendarLanguage", expressionAllowed = true)
protected final lazy val calendarLanguageRaw = requireProperty(optionCalendarLanguageRaw)
- protected final lazy val optionBinaryBooleanTrueRepRaw = findPropertyOption("binaryBooleanTrueRep")
+ protected final lazy val optionBinaryBooleanTrueRepRaw = findPropertyOption("binaryBooleanTrueRep", expressionAllowed = true)
protected final lazy val binaryBooleanTrueRepRaw = requireProperty(optionBinaryBooleanTrueRepRaw)
- protected final lazy val optionBinaryBooleanFalseRepRaw = findPropertyOption("binaryBooleanFalseRep")
+ protected final lazy val optionBinaryBooleanFalseRepRaw = findPropertyOption("binaryBooleanFalseRep", expressionAllowed = true)
Review comment:
I've updated the commit to remove the allowedExpression=true from the lines where it was incorrect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] IanCarlsonOwl commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
IanCarlsonOwl commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r489573729
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Fixed spelling of expression and changed comment syntax.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r493874522
##########
File path: daffodil-test/src/test/resources/org/apache/daffodil/section00/general/testExpressionPropertyWarnings.tdml
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<tdml:testSuite xmlns:tdml="http://www.ibm.com/xmlns/dfdl/testData"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:dfdl="http://www.ogf.org/dfdl/dfdl-1.0/"
+ xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:ex="http://example.com"
+ xmlns:fn="http://www.w3.org/2005/xpath-functions"
+ xmlns:daf="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:ext"
+ suiteName="test of warnings for when a property can't be an expression but looks like an expression"
+ defaultRoundTrip="onePass">
+
+ <tdml:defineSchema name="S1" elementFormDefault="unqualified">
+ <xs:include schemaLocation="org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" lengthKind="delimited"/>
+ <xs:element name="root" type="xs:double" dfdl:textNumberPattern="#####0.0#####" dfdl:textStandardInfinityRep="{../ex:thisCantBeAnExpression}" />
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="expressionPropertyWarning1" model="S1" root="root">
+ <tdml:document>1.0</tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <ex:root xsi:type="xs:double">1.0</ex:root>
Review comment:
Created DAFFODIL-2402
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r487308164
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #418: Add warnings for properties which look like expressions
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #418:
URL: https://github.com/apache/incubator-daffodil/pull/418#discussion_r487308164
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
Review comment:
remove blank line. Also above no space between ! and isForExpression
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
+ SDW( WarnID.NonExpressionPropertyValueLooksLikeExpression, "Property %s that looks like an expression cannot be an expression: %s", pname, v )
Review comment:
Line too long. Wrap args.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -120,7 +127,7 @@ trait FindPropertyMixin extends PropTypes {
val propCache = new scala.collection.mutable.LinkedHashMap[String, PropertyLookupResult]
- def findPropertyOption(pname: String): PropertyLookupResult = {
+ def findPropertyOption(pname: String, isForExpression: Boolean = false): PropertyLookupResult = {
Review comment:
Add scaladoc explaining purpose of isForExpression and that only for properties that can be expressions have to pass true.
Maybe the name should be "canBeExpressionOrNot" because many properties are optionally for expressions e.g, dfdl:byteOrder which can be literally "bigEndian" or can be an expression. Other properties like dfdl:inputValueCalc are always an expression, but we don't need this to be passed as true for them.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
Review comment:
Use // to introduce comments, preferentially to the /* */ pairs. Except for scaladoc/javadoc at top of methods/classes.
##########
File path: daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/PropertyScoping.scala
##########
@@ -131,6 +138,22 @@ trait FindPropertyMixin extends PropTypes {
lr
}
}
+ /* if this looks like it could be an epression, issue a warning */
+ /* only if isForExpression is false*/
+ /* for runtime mixins, need to supply true, other places, no change*/
+ if( ! isForExpression ) {
+ propRes match{
+ case Found( v, _, _, _ ) => {
+
+ if( v.startsWith( "{")) {
Review comment:
We should make this test a bit more selective, like check if it begins with double curly braces (which is an escape, so don't warn), and also check that it begins and ends with a single brace.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org