tuxji commented on code in PR #919:
URL: https://github.com/apache/daffodil/pull/919#discussion_r1091269491
##########
project/Rat.scala:
##########
@@ -38,35 +38,42 @@ object Rat {
// generated_code.[ch] examples
file("daffodil-runtime2/src/test/c/examples"),
+ // Apache Rat thinks these files are binary since the file name contains
".lib"
+
file("daffodil-schematron/src/main/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"),
+
file("daffodil-japi/src/test/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"),
+
file("daffodil-sapi/src/test/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"),
+
file("daffodil-lib/src/main/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"),
+
file("daffodil-lib/src/test/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"),
+
// test files that cannot include the Apache license without breaking tests
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/hextest.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input1.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input2.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input3.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input4.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input5.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input6.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input7.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input8.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input9.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input10.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input11.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input12.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input13.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input14.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input14.exi"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input14.exisa"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input15.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input16.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input18.json"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input18.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input18.exi"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input18.exisa"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input19.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/inputBig1M.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/prefix.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/test_DFDL-714.txt"),
-
file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/uuid.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/hextest.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input1.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input2.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input3.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input4.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input5.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input6.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input7.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input8.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input9.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input10.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input11.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input12.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input13.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input14.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input14.exi"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input14.exisa"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input15.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input16.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input18.json"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input18.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input18.exi"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input18.exisa"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/input19.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/inputBig1M.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/prefix.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/test_DFDL-714.txt"),
+
file("daffodil-cli/src/test/resources/org/apache/daffodil/cli/input/uuid.txt"),
file("daffodil-io/src/test/resources/iso8859.doc.dat"),
Review Comment:
Rat.scala was accidentally included in this pull request's changeset which
is breaking the CI since the refactoring needs to be committed together with
the Rat.scala changes. Remove Rat.scala from the changeset and make sure
git-restore.sh calls `git restore project`.
##########
project/OsgiCheck.scala:
##########
@@ -0,0 +1,112 @@
+/*
+ * 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.
+ */
+
+import java.io.File
+import sbt.MessageOnlyException
+import scala.collection.mutable.Map
+
+object OsgiCheck {
+ val prog = "osgiCheck"
+
+ // Directories in the repository root that are libraries which should be
checked
+ val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r)
+
+ // Only filenames that match here are considered
+ val fileMatcher = """.*\.(scala|java)""".r
+
+ // Ignore these libraries, packages, and directory names
+ var ignoreLibs = Seq(
+ "daffodil-macro-lib", // multi-package
+ )
+ val ignorePackages = Set("org.apache.daffodil")
+ var ignoreDirs = Set("test")
Review Comment:
OsgiCheck may want to ignore FieldFinder.scala since it contains only
comments (every line of code has been commented out):
```sbt
sbt:daffodil> osgiCheck
osgiCheck: warning:
./daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/reflection/FieldFinder.scala:
could not find package statement
osgiCheck: 0 error(s), 1 warning(s)
[success] Total time: 0 s, completed Jan 30, 2023, 3:14:06 PM
sbt:daffodil>
```
Futhermore, the refactoring moves FieldFinder.scala and its accompanying
test (which also has every line of code commented out) to a new place, but
fails to update their commented-out package statements:
```rg
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/reflection/FieldFinder.scala
24://package org.apache.daffodil.reflection
daffodil-runtime1/src/test/scala/org/apache/daffodil/runtime1/reflection/TestFieldFinder.scala
18://package org.apache.daffodil.reflection
```
##########
scripts/refactor/rename-dirs.sh:
##########
@@ -0,0 +1,152 @@
+#!/bin/bash -ex
+
+# 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.
+
+cd "$(dirname $0)/../.."
+SRC_ROOT=src/main/scala/org/apache/daffodil
+TEST_ROOT=src/test/scala/org/apache/daffodil
+
+function move {
+ local source_dir=$1
+ local target_dir=$2
+ local tmp=/tmp/daffodil-refactor
+
+ rm -rf $tmp
+ mkdir $tmp
+ mv $source_dir/* $tmp
+ rmdir $source_dir
+ mkdir -p $target_dir
+ mv $tmp/* $target_dir
+ rm -rf $tmp
+}
+
+function rename {
+ local lib_name=$1
+ local target_dir=$2
+ local source_suffix=$3
+ (
+ cd $lib_name
+ move $SRC_ROOT/$source_suffix $SRC_ROOT/$target_dir
+
+ if [ -e $TEST_ROOT/$source_suffix ]; then
+ move $TEST_ROOT/$source_suffix $TEST_ROOT/$target_dir
+ fi
+ )
+}
+
+rename daffodil-cli cli
+rename daffodil-core core
+rename daffodil-lib lib
+rename daffodil-macro-lib lib
+rename daffodil-runtime1 runtime1
+rename daffodil-runtime1-layers layers/runtime1 layers
+rename daffodil-runtime1-unparser unparsers/runtime1 processors/unparsers
+rename daffodil-tdml-processor processor/tdml tdml/processor
+
+mv daffodil-io/src/main/scala/org/apache/daffodil/processors \
+ daffodil-io/src/main/scala/org/apache/daffodil/io
+mv daffodil-cli/src/test/scala/org/apache/daffodil/cli/CLI/* \
+ daffodil-cli/src/test/scala/org/apache/daffodil/cli/cliTest
+rmdir daffodil-cli/src/test/scala/org/apache/daffodil/cli/CLI
+mv daffodil-cli/src/test/resources/org/apache/daffodil/CLI \
+ daffodil-cli/src/test/resources/org/apache/daffodil/cli
+find daffodil-cli/src/test/scala -name "*.scala" -exec sed -i
"s_/CLI/_/cli/_g" {} \;
+sed -i "s_/CLI/_/cli/_g" project/Rat.scala
+
+mkdir -p daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tak
+mv daffodil-tdml-lib/src/main/scala/org/apache/daffodil/Tak.scala \
+ daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tak/Tak.scala
+sed -i 's/package org.apache.daffodil/package org.apache.daffodil.tak/'
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tak/Tak.scala
+
+sed -i 's/package org.apache.daffodil.tdml/package
org.apache.daffodil.processor.tdml/'
daffodil-tdml-processor/src/main/scala/org/apache/daffodil/tdml/*.scala
+mv daffodil-tdml-processor/src/main/scala/org/apache/daffodil/tdml/*.scala
daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml
+rmdir daffodil-tdml-processor/src/main/scala/org/apache/daffodil/tdml
+
+rmdir daffodil-runtime1-unparser/$SRC_ROOT/processors
+
+move daffodil-tdml-processor/src/test/scala/org/apache/daffodil/tdml \
+ daffodil-tdml-processor/src/test/scala/org/apache/daffodil/processor/tdml
+sed -i 's/package org.apache.daffodil.tdml/package
org.apache.daffodil.processor.tdml/' \
+ daffodil-tdml-processor/src/test/scala/org/apache/daffodil/processor/tdml/*
+move daffodil-tdml-processor/src/test/java/org/apache/daffodil/tdml \
+ daffodil-tdml-processor/src/test/java/org/apache/daffodil/processor/tdml
Review Comment:
When I rebuild daffodil after the refactoring, the daffodil-tdml-lib jar
contains some processor API classes too:
```
org/apache/daffodil/tdml/processor/
org/apache/daffodil/tdml/processor/AbstractTDMLDFDLProcessorFactory.class
org/apache/daffodil/tdml/processor/TDML$.class
org/apache/daffodil/tdml/processor/TDML.class
org/apache/daffodil/tdml/processor/TDMLDFDLProcessor.class
org/apache/daffodil/tdml/processor/TDMLParseResult.class
org/apache/daffodil/tdml/processor/TDMLResult.class
org/apache/daffodil/tdml/processor/TDMLUnparseResult.class
```
Meanwhile, the daffodil-tdml-processor jar contains the processor
implementations, although they're actually in a tdml subpackage of the
processor package:
```
org/apache/daffodil/processor/tdml/DaffodilTDMLDFDLProcessor.class
org/apache/daffodil/processor/tdml/DaffodilTDMLParseResult.class
org/apache/daffodil/processor/tdml/DaffodilTDMLSAXErrorHandler.class
org/apache/daffodil/processor/tdml/DaffodilTDMLUnparseResult.class
org/apache/daffodil/processor/tdml/Runtime2TDMLDFDLProcessor.class
org/apache/daffodil/processor/tdml/Runtime2TDMLDFDLProcessorFactory.class
org/apache/daffodil/processor/tdml/Runtime2TDMLParseResult.class
org/apache/daffodil/processor/tdml/Runtime2TDMLUnparseResult.class
org/apache/daffodil/processor/tdml/TDMLDFDLProcessorFactory.class
org/apache/daffodil/processor/tdml/TDMLInfosetInputter.class
org/apache/daffodil/processor/tdml/TDMLInfosetOutputter.class
```
There is no conflict because tdml is under processor, but the organization
is somewhat surprising. Does it make sense to move the tdml.processor API
classes up into the tdml package since TDMLRunner.scala needs to call these
interfaces anyway? Then we can continue using the tdml.processor package for
the implementation classes in the daffodil-tdml-processor jar, which is a
little more obvious package name than processor.tdml.
For emphasis and to make what I'm talking about plainer, I'm proposing that
daffodil-tdml-processor should contain the following class names after all
refactoring and renaming has been done:
```
org/apache/daffodil/tdml/processor/DaffodilCTDMLDFDLProcessor.class
org/apache/daffodil/tdml/processor/DaffodilCTDMLDFDLProcessorFactory.class
org/apache/daffodil/tdml/processor/DaffodilCTDMLParseResult.class
org/apache/daffodil/tdml/processor/DaffodilCTDMLUnparseResult.class
org/apache/daffodil/tdml/processor/DaffodilTDMLDFDLProcessor.class
org/apache/daffodil/tdml/processor/DaffodilTDMLDFDLProcessorFactory.class
org/apache/daffodil/tdml/processor/DaffodilTDMLInfosetInputter.class
org/apache/daffodil/tdml/processor/DaffodilTDMLInfosetOutputter.class
org/apache/daffodil/tdml/processor/DaffodilTDMLParseResult.class
org/apache/daffodil/tdml/processor/DaffodilTDMLSAXErrorHandler.class
org/apache/daffodil/tdml/processor/DaffodilTDMLUnparseResult.class
```
That is, I'm proposing moving up the TDML processor API classes in
daffodil-tdml-lib to org/apache/daffodil/tdml/TDML*, renaming the
"TDMLImplementation.Daffodil" classes in daffodil-tdml-processor from TDML* to
DaffodilTDML*, and renaming the "TDMLImplementation.DaffodilC" classes in
daffodil-tdml-processor from Runtime2TDML* to DaffodilCTDML* which makes what
each group of classes does more obvious. We can discuss whether to include
some or all of those changes in your refactoring script or perform those
changes in a separate PR before or after we finish this PR and its refactoring
changes.
FYI, I also want to rename daffodil-runtime2 to daffodil-c-generator (again,
to make it more obvious what it does) and refactor Main.scala to allow it to
call code generators for multiple languages seamlessly. I can slip that PR in
before or after this PR too - the changes will be pretty orthogonal (only two
lines in your scripts mention runtime2 anyway, and you actually won't need
these two lines anymore if we continue using the tdml.processor package in
daffodil-tdml-processor):
```rg
scripts/refactor/fix-imports.scala
157: ("case TDMLImplementation.DaffodilC =>
\"org.apache.daffodil.tdml.processor.Runtime2TDMLDFDLProcessorFactory\"", "case
TDMLImplementation.DaffodilC =>
\"org.apache.daffodil.processor.tdml.Runtime2TDMLDFDLProcessorFactory\""),
scripts/refactor/add-imports.scala
123: ("org.apache.daffodil.tdml.processor._",
"daffodil-tdml-processor/src/main/scala/org/apache/daffodil/processor/tdml/Runtime2TDMLDFDLProcessor.scala"),
```
##########
scripts/refactor/git-restore.sh:
##########
@@ -0,0 +1,65 @@
+#!/bin/bash -ex
+
+# 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.
+
+BASE_DIR="$(dirname $0)/../.."
+SRC_ROOT=src/main/scala/org/apache/daffodil
+TEST_ROOT=src/test/scala/org/apache/daffodil
+
+cd $BASE_DIR
+
+function clean {
+ lib_name=$1
+ target_dir=$2
+
+ rm -rf $lib_name/$SRC_ROOT/$target_dir
+ rm -rf $lib_name/$TEST_ROOT/$target_dir
+}
+
+git restore daffodil-*
Review Comment:
Make sure project/Rat.scala is restored too.
--
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]
