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

fix: Run shellcheck on hledger-bar #2159

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

colindean
Copy link
Contributor

@colindean colindean commented Jan 22, 2024

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

bin/hledger-bar Outdated Show resolved Hide resolved
@simonmichael
Copy link
Owner

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
Copy link
Owner

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

simonmichael added a commit that referenced this pull request Jan 22, 2024
…2159]

Also, it's now more compliant with the no-color.org spec:

  Command-line software which adds ANSI color to its output by default
  should check for a NO_COLOR environment variable that, when present
  and not an empty string (regardless of its value), prevents the
  addition of ANSI color.

so one can now temporarily override $NO_COLOR=1 in the environment by
setting it empty: NO_COLOR= hledger ...
@colindean
Copy link
Contributor Author

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 bd8bd39 doesn't fix the problem.

@simonmichael simonmichael reopened this Jan 23, 2024
@simonmichael
Copy link
Owner

simonmichael commented Jan 23, 2024 via email

@colindean
Copy link
Contributor Author

Done!

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

@simonmichael
Copy link
Owner

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

I [encountered][1] 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 doubt 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

plus some extra, manual additions in the form of a shellcheck directive
to accept something that's a little abnormal for shellcheck but fine
here. The `set -o inherit_exit` was also recommended by shellcheck.

[1]: https://github.com/Homebrew/homebrew-core/actions/runs/7606843601/job/20713321881?pr=160590
@colindean
Copy link
Contributor Author

Rebased!

@simonmichael simonmichael merged commit 4faa381 into simonmichael:master Jan 23, 2024
1 check passed
@colindean colindean deleted the shellcheck-bar branch January 23, 2024 04:23
simonmichael added a commit that referenced this pull request Jan 28, 2024
…2159]

Also, it's now more compliant with the no-color.org spec:

  Command-line software which adds ANSI color to its output by default
  should check for a NO_COLOR environment variable that, when present
  and not an empty string (regardless of its value), prevents the
  addition of ANSI color.

so one can now temporarily override $NO_COLOR=1 in the environment by
setting it empty: NO_COLOR= hledger ...
adept pushed a commit to adept/hledger that referenced this pull request Mar 8, 2024
adept pushed a commit to adept/hledger that referenced this pull request Mar 8, 2024
…imonmichael#2159]

Also, it's now more compliant with the no-color.org spec:

  Command-line software which adds ANSI color to its output by default
  should check for a NO_COLOR environment variable that, when present
  and not an empty string (regardless of its value), prevents the
  addition of ANSI color.

so one can now temporarily override $NO_COLOR=1 in the environment by
setting it empty: NO_COLOR= hledger ...
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.

2 participants