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

use sh instead of bash in load_data_aoi.sh #774

Merged
merged 1 commit into from
May 22, 2022

Conversation

avalentino
Copy link
Member

@avalentino avalentino commented May 21, 2022

Use the correct shell in the shebang string

The mintpy/sh/load_data_aoi.sh script is supposed to work with a standard unix sh shell but it actually contain some statements using a bash specific syntax.

$ sh -n mintpy/sh/load_data_aoi.sh 
mintpy/sh/load_data_aoi.sh: 49: Syntax error: redirection unexpected

Reminders

  • Fix #xxxx
  • Pass Codacy code review (green)
  • Pass Circle CI test (green)
  • Make sure that your code follows our style. Use the other functions/files as a basis.
  • If modifying functionality, describe changes to function behavior and arguments in a comment below the function declaration.
  • If adding new functionality, add a detailed description to the documentation and/or an example.

@welcome
Copy link

welcome bot commented May 21, 2022

💖 Thanks for opening this pull request! Please check out our contributing guidelines. 💖
Keep in mind that all new features should be documented. It helps to write the comments next to the code or below your functions describing all arguments, and return types before writing the code. This will help you think about your code design and usually results in better code.

@avalentino
Copy link
Member Author

Just for my curiosity, what is the purpose of having the mintpy/sh/*.sh scripts inside the mintpy Python package folder?
It seems to me that none of them is used in the Python code, so probably it could be a good idea to move them to another place.
Do I miss something?

@yunjunz
Copy link
Member

yunjunz commented May 21, 2022

You are right. Only sh/plot_smallbaselineApp.sh was used before in the code, but not anymore. I like the idea of moving them to another place. Do you have any suggestions for it?

Copy link
Member

@yunjunz yunjunz left a comment

Choose a reason for hiding this comment

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

LGTM.

@avalentino
Copy link
Member Author

You are right. Only 'sh/plot_smallbaselineApp.sh` was used before in the code, but not anymore. I like the idea of moving them to another place. Do you have any suggestions for it?

Probably it could be a scripts folder in the project root.
If you need to install those scripts, you could use the scripts argument of setuptools.setup.
In that case, anyway, I recommend to prefix your scripts with e.g. mintpy- in order to make it easy to recognize mintpy related stuff in the global /usr/bin/ folder (on unix at least).

If you want I can prepare a patch for it, but it will be not before a week.
I will be traveling the next week.

@yunjunz
Copy link
Member

yunjunz commented May 22, 2022

Having them in a scripts folder sounds good to me. Since they are not used currently, we don't need to install them. One could easily find them in the repo if they want to use them independently. I will wait for your PR then.

@yunjunz yunjunz changed the title Use the correct shell in the shebang string use sh instead of bash in load_data_aoi.sh May 22, 2022
@yunjunz yunjunz merged commit 5405746 into insarlab:main May 22, 2022
@avalentino avalentino deleted the bugfix/shell branch May 24, 2022 14:47
yuankailiu pushed a commit to yuankailiu/MintPy that referenced this pull request Jun 2, 2022
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.

2 participants