Clean themes A-L #2315
Clean themes A-L #2315
Conversation
| @@ -85,7 +86,7 @@ IP_SEPARATOR=', ' | |||
|
|
|||
| function get_ip_info { | |||
| myip=$(curl -s checkip.dyndns.org | grep -Eo '[0-9\.]+') | |||
| echo -e "$(ips | sed -e :a -e '$!N;s/\n/${IP_SEPARATOR}/;ta' | sed -e 's/127\.0\.0\.1\${IP_SEPARATOR}//g'), ${myip}" | |||
| echo -e "$(ips | sed -e :a -e "\$!N;s/\\n/${IP_SEPARATOR}/;ta" | sed -e "s/127\\.0\\.0\\.1\\${IP_SEPARATOR}//g"), ${myip}" | |||
| } | |||
There was a problem hiding this comment.
good catch... did this even work before?
There was a problem hiding this comment.
I doubt it. But for both before and after my fix, when I tried to run it, I got really weird output (maybe my ips executable is not the right one?). I left the possibility of further fixes for later,.
There was a problem hiding this comment.
I think it worked before. Rather, this PR breaks the second sed pattern.
In the original code, the first sed expression converted the newlines to the literal ${IP_SEPARATOR}, and the second sed expression deleted the literal 127.0.0.1${IP_SEPARATOR}. I guess the literal ${IP_SEPARATOR} would finally be included in PS1, and Bash expands it on the prompt evaluation.
However, after the present change, I'm afraid that there would be multiple problems:
- The first
sedexpression converts the newlines to the value of the shell variableIP_SEPARATOR, but the secondsedexpression still tries to match the literal${IP_SEPARATOR}instead of matching the value of the shell variableIP_SEPARATOR. - When the user stores
/or something to the shell variableIP_SEPARATOR, the slash would be treated as the terminator of thesedoperators, and it causes problems. If one would like to use the value ofIP_SEPARATOR, one should properly quote the value ofIP_SEPARATORbefore embedding it in thesedexpression. - When the user stores strings looking like a prompt escape sequence or shell expansions, it would be expanded unexpectedly when
PS1is evaluated. If one would like to expand the value ofIP_SEPARATORwithin this function, one should additionally quote the value so that the prompt evaluation ofPS1would reproduce the value ofIP_SEPARATORliterally.
There was a problem hiding this comment.
Edwin, care to fix the last bits so we can sign up on the merge?
| @@ -170,7 +174,7 @@ function limited_pwd() { | |||
| # Replace $HOME with ~ if possible | |||
| RELATIVE_PWD=${PWD/#$HOME/\~} | |||
|
|
|||
| local offset=$((${#RELATIVE_PWD} - $MAX_PWD_LENGTH)) | |||
| local offset=$((${#RELATIVE_PWD} - MAX_PWD_LENGTH)) | |||
|
|
|||
| if [ $offset -gt "0" ]; then | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But I disagree that it should anticipate an arbitrary
IFSvalue - 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=$oldifsIn 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.
There was a problem hiding this comment.
But I disagree that it should anticipate an arbitrary
IFSvalue - 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.
Description
As per #1696, this cleans up various files by fixing ShellCheck errors.
This cleans 10 themes (and removes two entries from
clean_files.txtthat 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.shsuccessfully.Screenshots (if appropriate):
Types of changes
Checklist:
clean_files.txtand formatted it usinglint_clean_files.sh.