Skip to content

Conversation

@hyperupcall
Copy link
Contributor

@hyperupcall hyperupcall commented Jun 2, 2025

Description

As per #1696, this cleans up various files by fixing ShellCheck errors.

This cleans 10 themes (and removes two entries from clean_files.txt that no longer exist.

In the agnoster theme, I changed the "Git repository dirty check" to be more consistent with what I added in hawaii50 (to fix a ShellCheck issue).

Motivation and Context

As mentioned in #1696, this minimizes mistakes in the code.

How Has This Been Tested?

Ran ./lint_clean_files.sh successfully.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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.

Copy link
Contributor

@seefood seefood left a comment

Choose a reason for hiding this comment

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

90% there.

local offset=$((${#RELATIVE_PWD} - $MAX_PWD_LENGTH))
local offset=$((${#RELATIVE_PWD} - MAX_PWD_LENGTH))

if [ $offset -gt "0" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why the shellcheck doesn't complain about this unquoted $offset. Maybe it is because IFS is not modified inside this file and offset is ensured to contain a number in this context? However, the shell configuration should anticipate an arbitrary IFS value set outside the file by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've noticed that recent ShellCheck releases have become better at knowing when variables have a value that can only be a number. But I disagree that it should anticipate an arbitrary IFS value - the value deduction is just too handy and it covers 99% of setups.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I disagree that it should anticipate an arbitrary IFS value - the value deduction is just too handy and it covers 99% of setups.

I wouldn't argue that there would be a sane setup with IFS including numbers, but it is a valid usage to temporarily set IFS to a number, do something, and then revert it.

$ oldifs=${IFS-$' \t\n'}
$ IFS='blahblah'
$ do-something
$ IFS=$oldifs

In such a case, [ $offset -gt "0" ] may be broken temporarily for the third and fourth prompts.

Actually, one could simply rewrite it as ((offset > 0)), which is more obvious and easier to understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I disagree that it should anticipate an arbitrary IFS value - the value deduction is just too handy and it covers 99% of setups.

I was actually a bit confused about the point, but have you possibly misunderstood as if I thought ShellCheck should anticipate an arbitrary IFS? No, I don't think ShellCheck should anticipate an arbitrary IFS value. It is understandable that ShellCheck assumes the normal IFS when the script does not modify IFS. By the shell configuration in my original reply, I meant Bash-it (which is a configuration for interactive shell sessions), but I didn't mean ShellCheck. The situation with interactive shell settings are a bit different from that for normal shell scripts. The user has access to all the global variables, and the user may modify these at arbitrary time based on what the user wants to do. An interactive shell configuration should care about various possibilities for the commonly used variables, such as IFS.

@seefood seefood merged commit da80656 into Bash-it:master Jul 13, 2025
6 checks passed
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.

3 participants