hiroyuki-sato commented on code in PR #46682:
URL: https://github.com/apache/arrow/pull/46682#discussion_r2123247616
##########
ci/scripts/r_build.sh:
##########
@@ -24,6 +24,12 @@ build_dir=${2}
: "${BUILD_DOCS_R:=OFF}"
+if [ -z "${INSTALL_ARGS}" ] ; then
+ R_INSTALL_ARGS=()
+else
+ read -r -a R_INSTALL_ARGS <<< "$INSTALL_ARGS"
+fi
Review Comment:
Thank you for your comment.
Suggested change violates SC2153.
```bash
#!/bin/bash
R_INSTALL_ARGS=()
for arg in ${INSTALL_ARGS}; do
R_INSTALL_ARGS+=("${arg}")
done
```
```
for arg in ${INSTALL_ARGS}; do
^-------------^ SC2153 (info): Possible misspelling: INSTALL_ARGS
may not be assigned. Did you mean R_INSTALL_ARGS?
```
SC2153 can fix like this.
```bash
: "${INSTALL_ARGS:-}"
R_INSTALL_ARGS=()
for arg in ${INSTALL_ARGS}; do
R_INSTALL_ARGS+=("${arg}")
done
```
This version doesn't violate shellcheck on my machine.
But, I want to quote `${INSTALL_ARGS}` part.
It seems violates SC2086(need quoting).
(I don't know why this syntax passed shellcheck, by the way)
However It doesn't work well.
So, If we use this plan, I think the comment may help.
```bash
: "${INSTALL_ARGS:-}"
R_INSTALL_ARGS=()
# Do not Quote ${INSTALL_ARGS} like "${INSTALL_ARGS}"
for arg in ${INSTALL_ARGS}; do
R_INSTALL_ARGS+=("${arg}")
done
```
I don't know how to fix `${INSTALL_ARGS}` part SC2086 safe.
SC2086 meant about the original code.
Before #46663, The original message was
```
In ci/scripts/r_build.sh line 45:
${R_BIN} CMD INSTALL ${INSTALL_ARGS} arrow*.tar.gz
^------^ SC2086 (info): Double quote to prevent globbing and word
splitting.
^-------------^ SC2086 (info): Double quote to
prevent globbing and word splitting.
```
`read -r -a R_INSTALL_ARGS <<< "$INSTALL_ARGS"` has unfamiliar syntax, but
it is SC2086 safe.
I support your decision. Should we change `for` 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
