Skip to content

Make foreach-bundle a proper shell script#36

Merged
schlessera merged 1 commit into
wp-cli:mainfrom
szepeviktor:patch-1
Dec 8, 2022
Merged

Make foreach-bundle a proper shell script#36
schlessera merged 1 commit into
wp-cli:mainfrom
szepeviktor:patch-1

Conversation

@szepeviktor

@szepeviktor szepeviktor commented Aug 21, 2022

Copy link
Copy Markdown
Contributor

☣️ This is highly theoretic: mostly written in GitHub editor.

@szepeviktor

szepeviktor commented Aug 21, 2022

Copy link
Copy Markdown
Contributor Author

Should I include fetch-org?

@danielbachhuber

Copy link
Copy Markdown
Member

@szepeviktor Out of curiosity, what functional benefit does this provide?

@szepeviktor

szepeviktor commented Dec 5, 2022

Copy link
Copy Markdown
Contributor Author

@danielbachhuber This is the way one writes Bash scripts. This is the natural way.
I try to add pointers in separate comments.

Comment thread foreach-bundle
Comment on lines -3 to +6
BUNDLE_DEPENDENCIES=$(curl --silent https://raw.githubusercontent.com/wp-cli/wp-cli-bundle/master/composer.json -q | jq '.["require"] | keys[]' )
BUNDLE_DEPENDENCIES=$(echo "wp-cli/wp-cli-bundle $BUNDLE_DEPENDENCIES")
get_bundle_dependencies()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • functionality goes into functions
  • no variable reuse

Comment thread foreach-bundle
Comment on lines 1 to +4
#!/usr/bin/env bash
#
# Execute a command in each subfolder.
#

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inform the reader.

Comment thread foreach-bundle
fi
esac
done
while read -r dependency; do

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pipe a function's output into a while loop, for loops are a bit more dangerous

Comment thread foreach-bundle
Comment on lines +16 to +18
if [ ! -d "${repo_path}" ]; then
continue
fi

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bail out early: make the code flow nicely

Comment thread foreach-bundle

echo "--- ${repo_path}: ---"
cd "${repo_path}"
bash -c "$@"

@szepeviktor szepeviktor Dec 5, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never use "$@" alone

@danielbachhuber

Copy link
Copy Markdown
Member

Great, thanks for clarifying @szepeviktor 😊

Let's get a second opinion from @schlessera

@schlessera

Copy link
Copy Markdown
Member

Yes, the above sounds good to me. That script was just a quick hack I assembled and was never meant to be added as-is. But as we already have it included, we better make sure it is properly built.

@schlessera schlessera added this to the 2.0.1 milestone Dec 8, 2022
@schlessera schlessera merged commit 059c5c6 into wp-cli:main Dec 8, 2022
@szepeviktor

Copy link
Copy Markdown
Contributor Author

Thank you.

@szepeviktor szepeviktor deleted the patch-1 branch December 8, 2022 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants