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

Review of unix scripts #64

Closed
wants to merge 18 commits into from
Closed

Review of unix scripts #64

wants to merge 18 commits into from

Conversation

markusschuh
Copy link
Member

Resolve the warnings & implement most important recommendations of ShellCheck

@CLAassistant
Copy link

CLAassistant commented Apr 1, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

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

@markusschuh Thanks for this PR. Contains some great fixes and improvements. Also should finally allow spaces in installation paths. However, a few changes seem to introduce bugs and some might need to be discussed (maybe my knowledge of bash is just incomplete). Please have a look at my review comments. Thanks in advance.

scripts/src/main/resources/scripts/command/build Outdated Show resolved Hide resolved
scripts/src/main/resources/scripts/command/build Outdated Show resolved Hide resolved
scripts/src/main/resources/scripts/command/eclipse Outdated Show resolved Hide resolved
scripts/src/main/resources/scripts/command/eclipse Outdated Show resolved Hide resolved
scripts/src/main/resources/scripts/command/eclipse Outdated Show resolved Hide resolved
scripts/src/main/resources/scripts/command/ng Show resolved Hide resolved
scripts/src/main/resources/scripts/functions Outdated Show resolved Hide resolved
@hohwille
Copy link
Member

hohwille commented Apr 1, 2019

I also was not aware of ShellCheck before. Maybe we can even integrate it into travis build so we get warnings/errors if we do not follow best practices. However, only makes sense if it is not too strict.

@markschuh
Copy link

Best is to use ShellCheck even before code is commited, e.g. with help of the shellcheck vscode extension.

Different Install exists, on Windows it could be run native (installed via scoop), inside WSL (win10) or as a docker container.

ShellCheck can be configured to ignore specific checks, Still missing is an option to ignore via priority. (e.g. don't show recommendations)

TravisCI integration exists and shellcheck is meanwhile preinstalled there.

The only "challenge" for devon-ide is, that the shell scripts can't be selected by file ending, but need to be listed all by name. I guess, it would be more helpful for CI/CD and browser syntax highlighting, if the .sh ending were used. But this has new challenges during deployment, when the file endings will be removed there.

@hohwille
Copy link
Member

hohwille commented Apr 2, 2019

Awesome, but according to shellcheck I seem to be stupid:
vscode-shellcheck/vscode-shellcheck#50

hohwille added a commit that referenced this pull request Apr 2, 2019
* #73: fix for load_properties in bash
* #68: fixed
* #61: allow "devon bash" to open bash on windows
* #61: automatically call devon initialization when bash or zsh is opened
* #69: support uninstall
* #69: fixed quotation, also support upgrade
* #43: doc update
* #64: applied fixes from PR
@markschuh
Copy link

I have added your CRs ( except the postponed replacement of doGetVariable ). I'm unsure, what happens with this PR if I upgrade to lastet commit in my forked repo, so I do not yet push the merge from my local clone to remote. The conflicts still allow auto resolving - so I assume, the merge of pull request can be handled,

N.B: the vscode-shellsheck is a linter. And as some other linters I have seen - and with the default setting "onType" - it should underline a new entry line like echo $@. If not, I guess your shellcheck can't be found by the linter. Documentation may not be perfect - but I have seen much, much worser extensions on the vscode marketplace.

@hohwille
Copy link
Member

hohwille commented Apr 5, 2019

Even though this PR itself was not merged and has too many conflicts now, I worked your bug fixes, suggestions and improvements into the code. I will close this PR now. Thanks for your work, experience in bash, bugfixes and hints.

@hohwille hohwille closed this Apr 5, 2019
@hohwille hohwille mentioned this pull request Apr 5, 2019
hohwille added a commit that referenced this pull request Apr 5, 2019
* #66: #64: bash code style
* #87: fixed (was broken with #43)
@hohwille hohwille added this to the rejected milestone Aug 10, 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