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

Apply all current ShellCheck suggestions and add lint-shell.sh #1414

Merged
merged 4 commits into from
Dec 31, 2022

Conversation

kristapsk
Copy link
Member

Based on #1175, but also covers install.sh changes since then, all other shell scripts and adds ShellCheck linter script. Not adding this to CI for now. I'm thinking we could split off all the linting to separate job, don't see a point in running them under all OS / Python version combinations.

@kristapsk
Copy link
Member Author

@dmp1ce You were author of original PR, would be nice if you could take a look.

@PulpCattel
Copy link
Member

@kristapsk Is there a reason we want to write the linters (or other scripts) in shell rather than Python? Asking because I'm personally much more comfortable with Python and, well, the entire project is written in Python already. Some other reasons can be found at bitcoin/bitcoin#24783

It's not a big deal, I'm just curious if there are specific reasons.

@kristapsk
Copy link
Member Author

No specific reason. I based old lint-python.sh on one from Core, before they ported them to Python. Now for shellcheck also copied old one written in bash, to not use different languages here. Bet I'm not against somebody changing our linters to Python. Just don't care about that too much myself, haven't run into issues because of that.

install.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@dmp1ce dmp1ce left a comment

Choose a reason for hiding this comment

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

LGTM. My only question is about commenting out old code that should probably be removed.

@dmp1ce
Copy link
Contributor

dmp1ce commented Dec 29, 2022

@PulpCattel @kristapsk Unless issues exist with the shell scripts they should stay as is because converting them to Python will likely cause a bug or two. Shellcheck does a good job of finding potential issues with shell scripts.

@kristapsk
Copy link
Member Author

@PulpCattel @kristapsk Unless issues exist with the shell scripts they should stay as is because converting them to Python will likely cause a bug or two. Shellcheck does a good job of finding potential issues with shell scripts.

Agree there is no much need to change what already works. But if we do convert to Python, likely we can just copy and modify little bit existing converted scripts from Bitcoin Core. That would lower possibility of introducing new bugs. Their current lint-shell.py also uses ShellCheck, just calls it not from a bash script, but from a Python script.

@kristapsk kristapsk merged commit ea7a6e1 into JoinMarket-Org:master Dec 31, 2022
@kristapsk kristapsk deleted the shellcheck branch December 31, 2022 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants