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

Add option to append template to existing config #1723

Merged
merged 7 commits into from
Dec 11, 2020

Conversation

egvimo
Copy link
Contributor

@egvimo egvimo commented Dec 5, 2020

My first draft for the option to append template to existing config file in silent mode.

Closes #1720

@egvimo
Copy link
Contributor Author

egvimo commented Dec 5, 2020

What also bothers me a bit is that the two options no-modify-config and append-to-config are only used when the option silent is active.

It would be better, in my opinion, if the silent option is completely omitted and implicit by the other two options.

Maybe this can be achieved by another PR.

What do you think?

@NoahGorny
Copy link
Member

What also bothers me a bit is that the two options no-modify-config and append-to-config are only used when the option silent is active.

It would be better, in my opinion, if the silent option is completely omitted and implicit by the other two options.

Maybe this can be achieved by another PR.

What do you think?

We can always keep and append if the option is specified, even if in interactive mode. This will make more sense of the config. Not sure we want to make it implicit, I prefer more explicit approach, esp in installations

@egvimo egvimo marked this pull request as ready for review December 7, 2020 09:08
@egvimo
Copy link
Contributor Author

egvimo commented Dec 7, 2020

Now I have come to test my changes and write tests.

Unfortunately I'm getting a illegal option -- a error and I have no idea why. Did I miss some configuration?

@egvimo
Copy link
Contributor Author

egvimo commented Dec 10, 2020

I found my mistake and corrected it.

Now the feature can be merged, if the review is passed. ;)

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.

hey @egvimo
I left you a comment, lemme know what you think!

install.sh Outdated
@@ -148,6 +163,9 @@ if ! [[ $silent ]] && ! [[ $no_modify_config ]]; then
;;
esac
done
elif [[ $silent ]] && [[ $append_to_config ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

you can remove the while ! [ $silent ] check in line 150 in favor of while ! [ $append_to_config ], and remove [[ silent ]] check here. This will cause interactive mode to also automatically append if the option is specified

@egvimo
Copy link
Contributor Author

egvimo commented Dec 11, 2020

@NoahGorny I've done the changes from your comment.

@NoahGorny
Copy link
Member

Great job @egvimo !
You put a lot of effort into your PRs and I wanna say thank you for your help 😄
Also thanks @cornfeedhobo for the CR 😸

@NoahGorny NoahGorny merged commit 3019dc3 into Bash-it:master Dec 11, 2020
phreakocious pushed a commit to phreakocious/bash-it that referenced this pull request Dec 17, 2020
* Add option to append template to existing config

* Add test for append-to-config option

Co-authored-by: Egor Moor <egor.moor@edag-ps.com>
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.

Install silently but append template to .bashrc
3 participants