Fix more issues identified through shellcheck #355
Conversation
bash completions files are not supposed to have executable filemode set
shellcheck already relies on functionality added in bash v4.1, hence better to move the version check to the top of the script rather than checking in just one place also fixes proper invocation of 'read'
also fixes a rare bug when directory name contains a space
|
macOS users will have to update their bash versions, because the bash provided by apple is terribly out-of-date and will not be updated by apple anymore due to GPL licence concerns. Bash 4.1, which provides the necessary functionality, was released in 2010, which was 11 years ago. Updating is easy and most people who use the commandline (and hence todo.sh) will already have done so. This was also one of the main reasons why the hashbang line was changed to '#!/usr/bin/env bash' earlier, so that the correct bash is started on macOS instead of the apple-provided one. |
Breaks on macOS This reverts commit 17e3e88.
|
Thanks. That's again a whole lot of changes; most of it pretty straightforward. I wonder whether the consistent joining of Getting rid of the old Bash support on Mac OS would indeed be very useful; it's been a huge pain in the rear. I hope that your assessment that this is straightforward and most users would / can do it is correct. From the work on the mailing list, I know that at least for some people todo.sh is the only command-line application they use; some have installed Cygwin on Windows just for it. I think the CI build on Mac OS would have to be upgraded first, so that we don't lose the test coverage on that platform. |
needed due to old version of bash on macOS
|
I'm using this guide https://google.github.io/styleguide/shellguide.html#loops which recommends those exact changes for the for ... do. I'll revert the macOS changes and do a proper bash version check instead for that one read parameter. |
|
We could discuss dropping support for any version of Bash before 4.1 (2009-12-31). If we do, we'll release a major version upgrade to 3.0.0 for todo.txt-cli. References: |
|
If full compatibility with a locally installed updated bash version on a macOS were desired, then the hashbang #!/bin/bashwould have to be changed to #!/usr/bin/env bashin every test file. |
typeset is obsolete, replaced by local or declare also improve legibility of what is happening
$# will always be an integer and never needs quoting
|
I created two new branches that are ready for a pull request once this pull request is accepted. The first one https://github.com/a1346054/todo.txt-cli/tree/macos_tests fixes issues with tests not running with the correct bash on macOS (and possibly elsewhere), as well as improves the general code consistency. The second one https://github.com/a1346054/todo.txt-cli/tree/bash_syntax_tests further builds upon the first one by using more modern bash syntax, while still keeping backwards compatibility with older versions of bash. All tests pass, as well as my own testing. |
|
@a1346054 can you convert all files to use tabs and add an editorconfig file to the project as well? |
|
Sure, I'll set it up. Any other preferences besides tabs over spaces? |
Great question. I think it would be great to use shfmt as the formatter. That way we have a "standard" to follow. Could you set up shfmt in a new PR? Maybe set up sh-checker GitHub Action? |
|
I'd like to rework this PR and implement all the suggestions. Feel free to cherrypick the various commits though, or do whatever else is desired. |
We'll wait for your rework. Thanks again! |
|
@a1346054 Is this still in progress or can it be closed? |
|
It can be closed for now, there are many pending PRs and shfmt/shellcheck can be setup at any point once the PRs are merged. |
More issues were identified through shellcheck and reading through the code.
Continues work on #345 but more help would be appreciated because I'm not too familiar with the codebase - more shellcheck issues remain to be fixed.
All tests pass.