bash-it
bash-it copied to clipboard
Themes: Fix `shellcheck` (SC2154); Part C
Please review commit notes as appropriate.
Description
As a follow on to my work removing .shellcheckrc
(#1947) from hiding problems, this PR addresses these problems inside individual themes.
Motivation and Context
Due to the nature of themes, they use a huge number of variables from the themes lib and colors lib, but shellcheck
is unable to follow the code flow so can't tell if these variables are defined or not. This patchset is meant to improve code so that shellcheck
can either see the variables properly, or can trust that we have it under control.
NOTE: All the commits in this PR are single-file changes, so it may be easier to review per commit rather than all at once. I'm open to pulling some files out to separate PRs if needed.
How Has This Been Tested?
VIRTUAL_ENV= THEME_SHOW_CLOCK= RBFU_RUBY_VERSION= BASH_IT_THEME=teh_them bash -u
and all tests pass.
Types of changes
- [x] Bug fix (non-breaking change which fixes an issue)
Checklist:
- [x] My code follows the code style of this project.
- [x] If my change requires a change to the documentation, I have updated the documentation accordingly.
- [x] I have read the CONTRIBUTING document.
- [x] If I have added a new file, I also added it to
clean_files.txt
and formatted it usinglint_clean_files.sh
. - [x] I have added tests to cover my changes, and all the new and existing tests pass.
This is ready to review and hopefully ready to merge 😃
NOTE: All the commits in this PR are single-file changes, so it may be easier to review per commit rather than all at once. I'm open to pulling some files out to separate PRs if needed.
After going through all these themes, I have this suspicion that a lot of the themes are broken.
@davidpfarrell, @cornfeedhobo, @NoahGorny, I'd love to get some feedback on this one when any of you have time!
I've used ?
for everything that seems pretty rock-solid definitely already defined (e.g. colors), and :-
for everything that looks like a user-configurable preference (so everything still works no matter whether it exists or not). I've alsö run shfmt
using current project settings and added to clean_files.txt
. There shouldn't be any behavior changes at all (except perhaps if there were an unexpectedly failing conditional due to missing parameter).
A couple of things I've noticed:
- some of the logic in some themes is copy/pasta from another, with slight divergence
- some of the logic does not make sense to me; I don't understand what it's supposed to do.
- in a few places, variables are references which exist nowhere else in the codebase...
- something like half of the special characters in the themes show as the Unicode box on my system; is this normal? Is there a reference which shows them correctly (screenshots)?
It's very hard for me to try to maintain so many branches for so long, so if you have some suggestions which files look good I'd be happy to cherry-pick them out to a few branches. Thanks!
EDIT: I just opened #1977 with my best guess at an "easy" subset. Any suggestions which files you think really should be on there own, I'll be happy to split those out specifically. (essential
being one)
Alsö, rebased on current master.
@davidpfarrell, @NoahGorny, this PR is now much smaller since the intermediate PRs have been merged.
Hi @gaelicWizard, I think we should probably split this up for even more parts, this will make it much easier to review for me (the changes are unrelated to each other)
@NoahGorny, most of these already have open PRs. The "base" theme has #2038, pure has #2068, &c. I expect this PR to shrink even more as those are merged and leave only a few tidbits after.
This bad boy has shrunk down pretty small. Three files left: bargbuk, binaryanomaly, and modern. I think that's fair to review together.