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

Fix home dir substitution #1974

Merged
merged 3 commits into from
Oct 18, 2021
Merged

Fix home dir substitution #1974

merged 3 commits into from
Oct 18, 2021

Conversation

zou000
Copy link
Contributor

@zou000 zou000 commented Oct 17, 2021

See the discussions in 953e422#commitcomment-58148656 . Tested with bash 5.1 and bash 3.2

Description

Motivation and Context

How Has This Been Tested?

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.

See the discussions in 953e422#commitcomment-58148656 . Tested with bash 5.1 and  bash 3.2
davidpfarrell referenced this pull request Oct 17, 2021
The tilde should not have been escaped, and in fact I did not have it escaped in my main branch, but the PR I submitted did have it escaped and...now it shows up in the prompt line for all the PowerLine themes... oops.
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'm approving this since it does everything I asked :)

However there are at least 6 other occurrence of this idea in the repository that will also need attention at some point.

Thanks for taking the time @zou000 !

Copy link
Contributor

@gaelicWizard gaelicWizard left a comment

Choose a reason for hiding this comment

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

The quotes are necessary to avoid word splitting and will be rejected by the automated checks

@gaelicWizard
Copy link
Contributor

Is there a reason we're not using \w?

@zou000
Copy link
Contributor Author

zou000 commented Oct 17, 2021

The quotes are necessary to avoid word splitting and will be rejected by the automated checks

Can you elaborate? I tested with a directory name with space in it and it worked as expected.

❯mkdir t\ t
❯cd t\ t
❯cwd=${PWD/$HOME/\~}
❯echo $cwd
~/t t

@zou000
Copy link
Contributor Author

zou000 commented Oct 17, 2021

Is there a reason we're not using \w?

Hmm... no idea, I change to use \w it worked for both 5.1 and 3.2. Will update the PR

Copy link
Contributor Author

@zou000 zou000 left a comment

Choose a reason for hiding this comment

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

Use \w for pwd

@davidpfarrell
Copy link
Contributor

Is there a reason we're not using \w?

\w of course !

The quotes are necessary to avoid word splitting and will be rejected by the automated checks

Soo the quotes do not appear to be necessary in the context on an assignment:

test.sh

#!/usr/bin/env bash

# assign with no quotes
wd=${PWD/#$HOME/\~}
echo "${wd}"

# direct usage with no quotes
echo ${PWD/#$HOME/\~}

shellcheck

$ shellcheck test.sh 

In test.sh line 8:
echo ${PWD/#$HOME/\~}
     ^--------------^ SC2086: Double quote to prevent globbing and word splitting.

Notice that the assignment did not generate a warning.


/Side Note : @gaelicWizard I've been meaning to tell you that I came across you in the wild:

I was researching an issue for my work laptop (i.e not related to bash-it at all) and found your posts there.

I just thought it was cool and wanted to mention it ...

@gaelicWizard
Copy link
Contributor

rejected by the automated checks
Can you elaborate?

The Bash It repo is set up to run shellcheck on everything, and it's fairly pedantic about quoting variables (and can be made more pedantic, optionally), and I love it. I first began contributing code to Bash It because it wouldn't run on my system with a space in $BASH_IT, so it seems I may be the unofficial quoter-in-chief for the time being! 😃

Copy link
Contributor

@gaelicWizard gaelicWizard left a comment

Choose a reason for hiding this comment

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

Honestly, I feel kinda silly that I "fixed" the original by making it worse when the correct fix was to stop being clever entirely and just use the regular prompt string escapes 🤪

Thanks for fixing my "fix"!! ♥

@gaelicWizard
Copy link
Contributor

in the wild

I was way more of a prick back then, so feel free to look the other way... 😉

@NoahGorny
Copy link
Member

well done @zou000, and thanks for the review @gaelicWizard and @davidpfarrell 😄

@NoahGorny NoahGorny merged commit 5feebcc into Bash-it:master Oct 18, 2021
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.

4 participants