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

run shellcheck #97

Merged
merged 17 commits into from
May 14, 2021
Merged

run shellcheck #97

merged 17 commits into from
May 14, 2021

Conversation

ismith
Copy link
Contributor

@ismith ismith commented May 9, 2021

Fixes #64

ismith and others added 5 commits May 9, 2021 15:50
Co-authored-by: Conrad Ludgate <oon@conradludgate.com>
In src/shell/atuin.bash line 1:
export ATUIN_SESSION=$(atuin uuid)
       ^-----------^ SC2155: Declare and assign separately to avoid masking return values.
@ismith
Copy link
Contributor Author

ismith commented May 9, 2021

I'm not sure why it's still not picking up the new action. Any ideas, @conradludgate ?

@ismith ismith marked this pull request as ready for review May 9, 2021 22:52
@conradludgate
Copy link
Collaborator

https://github.com/ellie/atuin/actions/runs/826177834/workflow

@ismith
Copy link
Contributor Author

ismith commented May 9, 2021

D'oh! That's it!

ismith added 5 commits May 9, 2021 15:58
https://github.com/koalaman/shellcheck/wiki/SC1071, and the ignore: param
in ludeeus/action-shellcheck only supports _directories_, not _files_.
So instead, we manually add any error the shellcheck step finds in the
file to the above line ...
@ismith
Copy link
Contributor Author

ismith commented May 9, 2021

Ready for review. One thing to put eyes on: this 92a74ea is fine, right? We're just checking for a status code there?

@bl-ue
Copy link
Contributor

bl-ue commented May 10, 2021

Shellcheck + zsh: koalaman/shellcheck#809

@conradludgate
Copy link
Collaborator

Do you think you could add a comment to every ignore line?

@ismith ismith requested a review from conradludgate May 10, 2021 15:35
src/shell/atuin.bash Outdated Show resolved Hide resolved
Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

Thanks for this! I've just got one small suggestion (as it looks like the = should not be there?)

Happy to merge once this is done 😊

ismith and others added 2 commits May 12, 2021 18:40
Co-authored-by: Ellie Huxtable <ellie@elliehuxtable.com>
@ismith
Copy link
Contributor Author

ismith commented May 13, 2021

Thanks for catching that! I also resolved the merge conflict in install.sh.

@ellie
Copy link
Member

ellie commented May 14, 2021

Thank you! 🚀

@ellie ellie merged commit a127408 into atuinsh:main May 14, 2021
printf '\neval "$(atuin init zsh)"' >> ~/.zshrc

curl https://raw.githubusercontent.com/rcaloras/bash-preexec/master/bash-preexec.sh -o ~/.bash-preexec.sh
printf '\n[[ -f ~/.bash-preexec.sh ]] && source ~/.bash-preexec.sh' >> ~/.bashrc
# Use of single quotes around $() is intentional here
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they are. We want the string as is into the rc file.

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.

Use shellcheck on all shell scripts
5 participants