kbendick commented on a change in pull request #1227:
URL: https://github.com/apache/iceberg/pull/1227#discussion_r483927060
##########
File path: dev/source-release.sh
##########
@@ -66,7 +66,12 @@ tarball=$tag.tar.gz
# be conservative and use the release hash, even though git produces the same
# archive (identical hashes) using the scm tag
-git archive $release_hash --prefix $tag/ -o $tarball .baseline api arrow
bundled-guava common core data dev flink gradle gradlew hive mr orc parquet pig
spark spark2 spark-runtime spark3 spark3-runtime LICENSE NOTICE README.md
build.gradle baseline.gradle deploy.gradle tasks.gradle jmh.gradle
gradle.properties settings.gradle versions.lock versions.props version.txt
+adds=" .baseline" # prefixed with a blank space for each file name including
the first one
+excludes="build|examples|jitpack.yml|python|site" # excluded as they are not
of use for releasing jars
+echo WARNING! The following files/directories are to be excluded from git
archive: ${excludes}
+archives=$(ls | grep -vE ${excludes} | tr '\n' ' ')${adds}
Review comment:
Small nit / fyi: When I ran this, I wound up with an additional space
between everything generated from the ls / grep / tr and the $adds.
This is not a huge deal, but it might be cleaner to either split this into
two lines for readability or prepend the ${adds} and move the leading space
from adds above on line 69 to the end, e.g. `adds=".baseline "`.
I would personally suggest the two line approach (or an equivalent but
arguably less readable one line approach):
Two lines:
```bash
adds=".baseline" # notice there's no whitespace included
excludes="build|examples|jitpack.yml|python|site"
echo "NOTICE! The following files/directories are to be excluded from git
archive: ${excludes}"
includes=$(ls | grep -vE $excludes) # Where we're splitting the generation
of archives into two lines
archives=$(echo "${adds} ${includes}" | tr '\n' ' ') # Second line applies
the space between `adds` and `includes` and then normalizes both.
```
The equivalent one line solution would be
```bash
adds=".baseline"
....
archives=$(echo "${adds} $(ls | grep -vE ${excludes})" | tr '\n' ' ')
```
I'm personally more of a fan of the two line solution as it removes the need
for both a padded space (and the associated entire sentence commnent) in
`$adds` and I personally find it more readable.
However, this is just a nit / FYI and the current solution is fine (though
I'm a big fan of removing the comment from `$adds` and letting the variable
names be our comments as in the two line solution and also not requiring
special padding in the $adds variable). 🙂
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
