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

install: use .bashrc and notify user #2039

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

Conversation

gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Jan 13, 2022

Description

The logic to guess whether to use .bash_profile or .bashrc was buggy and wrong. Just use .bashrc and either automatically fill in a .bash_profile, or notify the user that they need to edit their .bash_profile.

This closes #1455, fixes #2084, and resolves #2022.

Motivation and Context

I honestly don't think that we should ever modify a user's ~/.bashrc or ~/.bash_profile, but if we do then we should be better about it.

TODO: add an optional pass to _bash-it-migrate which moves ~/.bash_profile to ~/.bashrc if the latter doesn't exist.

How Has This Been Tested?

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 January 13, 2022 05:43
install.sh Outdated Show resolved Hide resolved
docs/installation.rst Outdated Show resolved Hide resolved
@gaelicWizard gaelicWizard force-pushed the lib/install branch 3 times, most recently from 18196a6 to 1bcc3f4 Compare February 9, 2022 06:01
@gaelicWizard gaelicWizard force-pushed the lib/install branch 4 times, most recently from 3efc183 to 7fd8deb Compare February 12, 2022 07:05
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

Hi @gaelicWizard, left a couple of comments

uninstall.sh Outdated Show resolved Hide resolved
uninstall.sh Outdated Show resolved Hide resolved
@gaelicWizard gaelicWizard force-pushed the lib/install branch 8 times, most recently from 90fcb4d to eb14b34 Compare February 15, 2022 21:21
@gaelicWizard
Copy link
Contributor Author

Anyway, I've just learned that fgrep is deprecated. Should we alsö kill _bash-it-grep()?

@gaelicWizard gaelicWizard force-pushed the lib/install branch 3 times, most recently from 0f14c28 to 118861f Compare February 20, 2022 20:33
@gaelicWizard gaelicWizard force-pushed the lib/install branch 4 times, most recently from 69dd2fe to 2f7da32 Compare March 4, 2022 20:50
@gaelicWizard
Copy link
Contributor Author

Ok, there should now be tests covering both cases of existing ~/.bashrc or ~/.bash_profile.

gaelicWizard and others added 9 commits March 4, 2022 13:04
The logic to guess whether to use `.bash_profile` or `.bashrc` was buggy and wrong. Just use `.bashrc` and either automatically fill in a `.bash_profile`, or notify the user that they need to edit their `.bash_profile`.
docs/installation: add to note about interactive/login shells
- Alsö, add implementation note at top.
uninstall: TIL that `fgrep` is deprecated...
- and generally comment out useless varbls
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

Hi @gaelicWizard
sorry for the long time for the review, but I think it looks good.
@davidpfarrell - wanna take a look?

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