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.sh] add flag not to modify exist config file during installation #810

Merged
merged 2 commits into from
Nov 7, 2016

Conversation

knorth55
Copy link
Contributor

What is changed

  • ask whether you want to modify existing config file (i.e. .bashrc) during installation

there is no way to run install.sh, keeping config file (i.e. .bashrc) unchanged.
therefore, i add this option for interactive mode.

@nwinkler
Copy link
Member

What is the use case for this? Isn't the purpose of the installation to enable Bash-it by including it in the config file that is run when you start a new shell?

I'm not sure I understand how someone would use Bash-it in this case.

@knorth55
Copy link
Contributor Author

knorth55 commented Oct 25, 2016

i manage config files on github and, in my config file, bash_it config is already written.
so there is no need to modify config in my case.
when i install bash_it to new computer, i only want to select which completions or plugins should be installed by install.sh

@nwinkler
Copy link
Member

OK, got it. In that case - wouldn't it be smarter to detect whether the config file already contains the required lines that load Bash-it?

I would like to avoid having lots of questions in the installer if possible, especially when many of the questions aren't required for the majority of users.

@knorth55
Copy link
Contributor Author

knorth55 commented Oct 26, 2016

Current problem is that there is no way to run the install script while keeping config file unmodified.
For interactive mode, the script should ask to modify it first, and question about backup files questions should be after that. (please see flow chart below)

i think it is not a long flow for installation, and, in the case that program modify file, it should ask.
unless you don't choose interactive mode, install script does not ask anything as before.

chart

@nwinkler
Copy link
Member

Yes, I get your use case, but I still feel that adding an additional question is too invasive, as for most users, the answer will be "Yes, please modify the file." Answering "No" will be a complete edge case, and I don't expect to see that frequently used.

Like I said, a much better option would be to check whether the config file already contains the required lines. Instead of asking the user, the install script would figure it out on its own. In your case, it would see that your ~/.bashrc already contains the lines that load Bash-it and would modify the file.

What do you think about that?

@knorth55
Copy link
Contributor Author

i always want to say 'No, do not modify it' to install script, but it is my case.
i don't know other users experience much.

for scraping config file, what you mean 'required line' is source $BASH_IT/bash_it.sh
but i don't write config file like that, and a lot of users are same as me, they wrote config file by themselves.
therefore, i think checking config file is not a good idea.

i think whenever program modify existing file, they should ask it or tell it to user.
now install script modify config file while not telling anything to user (just telling existing backup file), and it is really weird for me
https://github.com/Bash-it/bash-it/blob/master/template/bash_profile.template.bash

@nwinkler
Copy link
Member

Thanks for the additional information, this makes it a lot easier to understand your use case.

Based on what you wrote, maybe using the install script is not a good idea. The primary purpose of the install script is to ensure that Bash-it is loaded when the user starts a new shell.

Out of curiosity, if you're not using a line like the one you quoted, how do you load Bash-it from your ~/.bashrc?

Also, when you run the install script, do you use it in interactive mode, so that it asks you about every plugin to enable? Or how do you use the installer? There might be better options for you to achieve the same thing.

I think your use case would better be covered by an additional command line flag to the install script. If the flag is provided, it doesn't modify the config file. How about that?

@knorth55
Copy link
Contributor Author

i rewrite alias with my own alias files so that i make bash code not to source bash_it alias

when i get new computer, i often run install script with interactive mode and delete generated bashrc.
flag is also good for me.

@nwinkler
Copy link
Member

Since I keep my dotfiles in a Git repo, I don't run the installer when setting up a new machine - I simply clone my dotfile repo and enable the Bash-it components that I need. The bashrc file already contains the lines that load Bash-it - no need to do that again, same as you.

I keep a script in my dotfiles repo that clones Bash-it and then enables what I need - maybe that would be useable approach for you as well? Here's the script in case you're interested: https://github.com/nwinkler/dotfiles/blob/master/install_bash_it.sh

Feel free to update the PR with a change that implements your feature through a flag instead of a question.

@knorth55
Copy link
Contributor Author

cool example script!
thank you :)
i will modify this PR to flag version

@nwinkler
Copy link
Member

Sure, you're welcome. Looking forward to the updated PR!

@knorth55 knorth55 force-pushed the install-modify-config branch from c3db83e to 6b07098 Compare October 31, 2016 09:39
@knorth55
Copy link
Contributor Author

i updated to use flag --no-modify-config

@knorth55 knorth55 changed the title [install.sh] ask to modify exist config file during install [install.sh] add flag not to modify exist config file during install Oct 31, 2016
@knorth55 knorth55 changed the title [install.sh] add flag not to modify exist config file during install [install.sh] add flag not to modify exist config file during installation Oct 31, 2016
@knorth55
Copy link
Contributor Author

knorth55 commented Nov 3, 2016

@nwinkler friendly ping. if you have time, can you review?

@nwinkler
Copy link
Member

nwinkler commented Nov 3, 2016

Sorry about the delay - I've been pretty busy. I'll try to find time today...

@knorth55
Copy link
Contributor Author

knorth55 commented Nov 3, 2016

thank you :) i'm not in hurry, so it's up to you

@nwinkler
Copy link
Member

nwinkler commented Nov 4, 2016

Sorry for the delay, finally had time to review this. Some findings:

  • Works fine when running the installer using either ./install.sh --no-modify-config or ./install.sh --interactive --no-modify-config. In these cases, the config file is not modified.
  • When running the installer using ./install.sh --silent --no-modify-config, the config file is modified. Could you please fix that use case? The expectation is that the config file is not modified in that case.
  • The README.md file should be updated to show the new installer option. (I know it's also missing a description of the --silent option - feel free to add that as well if you like, otherwise I'll add it in a separate update.)

Could you please take a look at these two items? Looks great otherwise.

@knorth55
Copy link
Contributor Author

knorth55 commented Nov 4, 2016

thank you for quick review, i will fix it :)

@knorth55 knorth55 force-pushed the install-modify-config branch from 6b07098 to 89bb325 Compare November 4, 2016 08:46
@knorth55 knorth55 force-pushed the install-modify-config branch from 89bb325 to ce65f57 Compare November 4, 2016 08:47
@knorth55
Copy link
Contributor Author

knorth55 commented Nov 4, 2016

i fixed bug and i add description in README.

@nwinkler nwinkler merged commit 71b7f95 into Bash-it:master Nov 7, 2016
@nwinkler
Copy link
Member

nwinkler commented Nov 7, 2016

Thanks - works fine now! Thanks again for hanging in there and accepting a compromise. If only more people would be like that 😄

@knorth55
Copy link
Contributor Author

knorth55 commented Nov 7, 2016

thank you so much for reviewing many times 😄

@knorth55 knorth55 deleted the install-modify-config branch November 7, 2016 08:21
BarbUk pushed a commit to BarbUk/bash-it that referenced this pull request May 8, 2017
[install.sh] add flag not to modify exist config file during installation
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.

2 participants