Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

spell/typos+quotes to avoid globbing/wordsplitting#5

Merged
geekcomputers merged 2 commits intomastergeekcomputers/Shell:masterfrom
unknown repositoryCopy head branch name to clipboard
Jan 3, 2023
Merged

spell/typos+quotes to avoid globbing/wordsplitting#5
geekcomputers merged 2 commits intomastergeekcomputers/Shell:masterfrom
unknown repositoryCopy head branch name to clipboard

Conversation

@ghost
Copy link

@ghost ghost commented Jan 2, 2023

fixing some spelling errors and adding double quotes to avoid globbing or word splitting

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hello. These are just some thoughts I had which might be of use. I hope they at least gives you a different perspective.

funct_check_params() # Start of the procedure
{
if [ ${NARG} -lt 3 ]; then # Check there are 3 arguments passed to the script, if not display the help
if [ "${NARG}" -lt 3 ]; then # Check there are 3 arguments passed to the script, if not display the help
Copy link

Choose a reason for hiding this comment

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

This change shouldn't be needed, because the variable should be an integer. Rather than forcing this within the test, I would make the script more robust by first ensuring it's an integer and catching when it's not.

# This will carry out the rename

for file in $(ls $work_dir/*$old_ext); do mv $file `echo $file | sed 's/\(.*\.\)'$old_ext_cut'/\1'$new_ext_cut/` ; done
for file in $(ls "$work_dir"/*"$old_ext"); do mv "$file" $(echo "$file" | sed 's/\(.*\.\)'"$old_ext_cut"'/\1'"$new_ext_cut"/) ; done
Copy link

Choose a reason for hiding this comment

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

It's almost never recommended or necessary to use ls(1) in a shell script, largely because glob filename pattern matching more than suffices, is more reliable, and is more efficient. In this case, you should be able to:

  for file in "$work_dir"/*"$old_ext"; do ...

Confirm first, in-case there's something unusual I'm overlooking.

Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

legacy backticks for $(...) notation

@ghost ghost self-requested a review January 3, 2023 02:51
@ghost
Copy link

ghost commented Jan 3, 2023

I think it's important to note that the use of backticks or graves for command substitution is not going anywhere any time soon, as far as I know. It may be syntax from days of old, but I believe it still holds relevance.

Variable=`some_command`

In the above example, there is no need for the $(...) syntax. No nesting of command substitution is required, and there are no quotes to confuse between the graves. The added character of the newer notation is therefore redundant.

However,

Variable=$(some_command "string $(other_command)")

In the above example, using the newer syntax makes sense, because it avoids the confusion between the final double-quote and the right-most grave, and nesting is required.

This is just my stance on command substitution syntax in shell programming. I use both, where appropriate.

Actually, there is another important argument for the newer method: many claim that the newer syntax is simply easier to see, regardless of what's next to it. However, this largely seems dependent on the user's font. I believe each user should always have a font suitable for such text, so that each character is sufficiently distinguishable from the rest.

@geekcomputers geekcomputers merged commit 34ec479 into geekcomputers:master Jan 3, 2023
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.

1 participant

Morty Proxy This is a proxified and sanitized view of the page, visit original site.