simonmichael / hledger

Robust, fast, intuitive plain text accounting tool with CLI, TUI and web interfaces.
https://hledger.org
GNU General Public License v3.0
2.85k stars 307 forks source link

fix: Run shellcheck on hledger-bar #2159

Closed colindean closed 5 months ago

colindean commented 5 months ago

I encountered an unexpected error when testing hledger-bar for proposed inclusion in Homebrew's default installation of hledger:

/opt/homebrew/Cellar/hledger/1.32.2_1/bin/hledger-bar: line 81:
conditional binary operator expected

I'm doubtful that this is fixed by running shellcheck, but checking a script against shellcheck is usually one of the first things I check before debugging shell!

This patch is ~generated by shellcheck with

shellcheck --shell=bash --enable=all --format=diff bin/hledger-bar | \
git apply
simonmichael commented 5 months ago

Thanks @colindean ! I never knew shellcheck could do that.

When I check it in master there's one shellcheck warning:

In bin/hledger-bar line 106:                                                                                                                                                                
cmd="hledger balance -Ocsv --transpose -N -0 --layout=bare -M $@"                                                                                                                           
    ^-- SC2124 (warning): Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.                                                                            

which I've just pushed a fix for.

The homebrew error seems like something caused by a different shell version, but I'm not sure what it could be. Testing here with shellcheck -s bash or ksh reports no warnings, sh or dash report a ton of warnings, more than what you're seeing. My bash here is 5.2.21.

simonmichael commented 5 months ago

Found it! Now fixed in master. hledger-bar was broken if $NO_COLOR was not defined. Thanks for the bug report.

colindean commented 5 months ago

Thanks @colindean ! I never knew shellcheck could do that.

It's a great tool, opinionated in that it never itself can modify code under inspection, unlike many if not most other linters. There are some idiosyncracies of doing shellcheck --format=diff | git apply but it works most of the time.

I've got something like this in many of my projects including shell files:

.PHONY: format-sh
format-sh: $(SHELL_FILES)
    $(SHELLCHECK) --shell=bash --enable=all --format=diff $(SHELL_FILES) | git apply

When I check it in master there's one shellcheck warning:

You'll see more if you pass --enable=all. This PR addressed all of them, which were all autocorrectable.

The homebrew error seems like something caused by a different shell version, but I'm not sure what it could be. Testing here with shellcheck -s bash or ksh reports no warnings, sh or dash report a ton of warnings, more than what you're seeing. My bash here is 5.2.21.

I'll have to dig into it a little more if the next hledger release to include bd8bd393f24cbef158763fbc902e46fc6584477a doesn't fix the problem.

simonmichael commented 5 months ago

You'll see more if you pass --enable=all. This PR addressed all of them, which were all autocorrectable.

Aha. If you care to repeat this for latest master and re-push, I'll merge it.

colindean commented 5 months ago

Done!

Shellcheck doesn't like the bare ${NO_COLOR} check, preferring -n.

simonmichael commented 5 months ago

Would you mind rebasing it to one clean commit. (git rebase locally, then git push -f)

colindean commented 5 months ago

Rebased!