stevedlawrence commented on a change in pull request #681:
URL: https://github.com/apache/daffodil/pull/681#discussion_r752461365
##########
File path: .gitattributes
##########
@@ -13,4 +13,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+# Do not include KEYS in archived source releases
/KEYS export-ignore
+
+# Do not convert to CRLF on Windows since tests require LF files
+/**/runtime2/examples/** text=auto eol=lf
Review comment:
Another thought on the test approach, if the example files are out of
date, the first time you run tests you'll get a failure, but the second time
the test will pass, even though the user hasn't changed anything. That feels
like potentially confusing behavior. And I think that's because a test is
updated the expected results. It feels wrong tests to be updating expected
results.
I'm fine doing something other than the `genExamples` approach if you think
it's too slow--daffodil-cli/stage does take some time, especially when making
updates. But I'm not sure tests are the right place for it.
Whitespace issue: If we choose to update .gitattributes to have
daffodil-runtime2/ to disable autocrlf, then I think we should just do it for
everything. Having some source that is always LF and some that is OS dependent
feels wrong. That said, as dumb as I think Git's decision is to enable
autocrlf, I also don't want to make development harder for people on windows.
Maybe there are some tools that really do need this autocrlf feature, so I'm
not sure disabling it for .scala files is safe? I'm not really sure...
One last thought regarding whitespace, we could minimize code changes by
adding some implicit classes:
```scala
implicit class StringNewLine(s: String) {
def stripMarginNL: String = s.stripMargin.replace("\r\n",
"\n").replace("\n", System.lineSeparator)
}
implicit class SeqNewLine[T](s: Seq[T]) {
def mkStringNL: String = s.mkString(System.lineSeparator)
}
```
Then multiline strings become something like
```scala
val foo =
"""|multi
|line
|string""".stripMarginNL
```
and Seqs become
```scala
val bar = Seq("multi", "line", "string").mkStringNL
```
There would need to be a big change to switch it over, but the new code
would look fairly clean and would ensure consistent newlines.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
