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

Bash isn't portable, use POSIX sh instead #560

Merged
merged 2 commits into from
Jun 13, 2023
Merged

Conversation

7heo
Copy link
Contributor

@7heo 7heo commented Jun 12, 2023

Also, hardcoding bash to /bin/bash is even less portable.

In addition, I commented an unused variable (name not present anywhere in the code base, not exported).

@datWeazel
Copy link

Why comment the unused variable? If truly unused removal should be okay. Commenting implicates that it might be relevant in certain conditions which might lead other devs to search for the "when is it relevant".

@7heo
Copy link
Contributor Author

7heo commented Jun 12, 2023

Why comment the unused variable? If truly unused removal should be okay.

I am not the original author of this script, it's not for me to decide. Maybe they will remember what they wanted to do with it and finish it. If I remove it, they might forget entirely.

@twizmwazin
Copy link
Contributor

I say just remove it, I don't think there was some larger intent here.

@7heo
Copy link
Contributor Author

7heo commented Jun 12, 2023

I mean, anyway, @dessalines wrote that line according to the git blame, so he will say something if it's meant to stay.

@7heo 7heo force-pushed the main branch 2 times, most recently from 187c808 to af77522 Compare June 12, 2023 22:35
Also, hardcoding bash to /bin/bash is even less portable.

In addition, I removed an unused variable (name not present anywhere in the
code base, not exported) and the related comments.
set -e

port=$1

Copy link
Member

Choose a reason for hiding this comment

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

Was fine to remove, its unecessary.

@dessalines dessalines merged commit 5b8bc23 into LemmyNet:main Jun 13, 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.

4 participants