Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace backticks (``) with $() for command substitution #2595

Merged
merged 9 commits into from
Jan 30, 2020
Merged

Replace backticks (``) with $() for command substitution #2595

merged 9 commits into from
Jan 30, 2020

Conversation

anbj
Copy link
Contributor

@anbj anbj commented Jan 30, 2020

Replace the legacy syntax of backticks (``) with the preferred $() for command substitution.

@PaulWessel
Copy link
Member

I had to learn about the legacy/obsoleteness of backticks first here. I suppose this is progress. Would you agree, @GenericMappingTools/core ?

@anbj
Copy link
Contributor Author

anbj commented Jan 30, 2020

See also https://mywiki.wooledge.org/BashFAQ/082.

@seisman
Copy link
Member

seisman commented Jan 30, 2020

Looks good to me if all the tests still pass, but csh may not support $() sytnax.

@PaulWessel
Copy link
Member

Only csh script we have is gmt_aliases.csh and it has
set gmt_modules = (gmt --show-classic)
which fails if we do

set gmt_modules = $(gmt --show-classic)
Illegal variable name.

So we can leave that as is. No other csh syntax is shown or used anywhere. We do not want to encourage csh use anyway.

@PaulWessel
Copy link
Member

Hi @anbj, did you run all the tests to see if there were any new failures?

@anbj
Copy link
Contributor Author

anbj commented Jan 30, 2020

No, sorry - not yet. I can run the tests tomorrow. I can also revert the changes for gmt_aliases.csh.

@seisman
Copy link
Member

seisman commented Jan 30, 2020

I just ran all tests locally. There are 4 failing tests:

	146 - anim01/anim01.sh (Failed)
	147 - anim02/anim02.sh (Failed)
	151 - anim06/anim06.sh (Failed)
	287 - gmtregress/draper.sh (Failed)

doc/examples/anim01/anim01.sh Outdated Show resolved Hide resolved
doc/examples/anim02/anim02.sh Outdated Show resolved Hide resolved
doc/examples/anim06/anim06.sh Outdated Show resolved Hide resolved
doc/examples/anim06/anim06.sh Outdated Show resolved Hide resolved
doc/examples/anim06/anim06.sh Outdated Show resolved Hide resolved
doc/examples/anim06/anim06.sh Outdated Show resolved Hide resolved
@anbj
Copy link
Contributor Author

anbj commented Jan 30, 2020

Great @seisman , thanks for catching these!

test/gmtregress/draper.sh Outdated Show resolved Hide resolved
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@PaulWessel PaulWessel merged commit f503d4a into GenericMappingTools:master Jan 30, 2020
@PaulWessel
Copy link
Member

Thanks for this work @anbj !

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