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

Themes: Fix shellcheck (SC2154); Part C #1954

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Sep 19, 2021

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

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@gaelicWizard gaelicWizard marked this pull request as ready for review September 19, 2021 06:41
@gaelicWizard gaelicWizard changed the title DRAFT: Fix SC2154 from themes Fix SC2154 from themes Sep 19, 2021
@gaelicWizard
Copy link
Contributor Author

gaelicWizard commented Sep 19, 2021

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.

@gaelicWizard gaelicWizard changed the title Fix SC2154 from themes Themes: Fix SC2154 Sep 19, 2021
@gaelicWizard
Copy link
Contributor Author

After going through all these themes, I have this suspicion that a lot of the themes are broken.

@gaelicWizard
Copy link
Contributor Author

@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)?

Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

I find this PR to be too large and risky - Although not optimal, I might like to see per-theme PR's here.

Or maybe make a few bigger PRs with easy to review changes and leave the more complex changes for single-theme PRs ...

themes/barbuk/barbuk.theme.bash Outdated Show resolved Hide resolved
plugins/available/alias-completion.plugin.bash Outdated Show resolved Hide resolved
@gaelicWizard gaelicWizard changed the title Themes: Fix SC2154 Themes: Fix shellcheck (SC2154) Oct 19, 2021
@gaelicWizard
Copy link
Contributor Author

@davidpfarrell, @NoahGorny, this PR is now much smaller since the intermediate PRs have been merged.

@NoahGorny
Copy link
Member

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)

@gaelicWizard
Copy link
Contributor Author

@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.

Handle all unbound parameters, even colors!

Alsö, fix some local variables and variable assignments.

The unicode/emoji symbols don’t show up for me so that makes me think they won’t work, but it *looks* like the history has these characters in them so…I dunno
Handle all unbound parameters, even colors!

Local some variables, &c
Handle all unbound parameters, even colors!
@gaelicWizard
Copy link
Contributor Author

This bad boy has shrunk down pretty small. Three files left: bargbuk, binaryanomaly, and modern. I think that's fair to review together.

@seefood seefood self-assigned this Nov 7, 2024
@seefood
Copy link
Contributor

seefood commented Nov 7, 2024

I'll gladly review it if you could please merge the latest master, there are some conflicts in barbuk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants