feature(install): automatically add alias on conflict command #11

alvinmatias69 · 2019-10-28T19:25:45Z

On some shell (especially zsh), g is already used or aliased for other
command. Prompt user to enter new alias for g on installation.

fix #10

stefanmaric

Hey @alvinmatias69

Thanks for taking your time.

I noticed a few things and I will slowly post feedback as I get the chance.

bin/install

stefanmaric

@alvinmatias69 thanks for your patience. I had a steep return to work.

I left a single note, but we need to take a diff direction UX-wise:

We need to collect the information about aliases on the selected shells before hand, and ask for approval and the chosen name before hand, before installing g and whatsoever.

Also, in case there are more than one selected shell with an alias conflict, we prompt once and use the same confirmation and alias to configure all the selected shells.

This is probably a very edge case (an user with two diff shells, having both the g alias), but it is also very odd to have the user type, let's say ggovm in one shell and g in other.

So to summarize:

We collect info about conflicting aliases in any of the selected shells, before hand
Before proceeding with the installation/upgrade, we prompt for confirmation to setup alternative name for g (confirmation, then choose the name)
We configure the alias in every shell selected for installation, no matter if some of the selected shells don't need it

PD: I pushed some stuff I had lingering around to the next branch. You would ideally rebase your branch on top of it.

Let me know if you have any questions.

bin/install

stefanmaric · 2019-12-09T17:57:34Z

Hey @alvinmatias69 , were you able to make some progress here? If you need advise or have doubts, don't hesitate to ping me.

If in other hand, it gets too complicated or you don't have the time to keep working on it, I can put some time later this week and take it from here.

Let me know!

alvinmatias69 · 2019-12-10T01:31:04Z

hey, I'm sorry for the long wait. It's been hectic at work lately. I've been working on it tho, but I still need some time to finish this. I'm truly sorry

stefanmaric · 2019-12-10T16:34:28Z

No worries @alvinmatias69, since you have work in progress, take your time to finish up. Let me know if you have doubts. <3

alvinmatias69 · 2019-12-10T18:26:48Z

Hi @stefanmaric, I've made some changes regarding our discussion above. The install scripts will check every selected shell to makes sure that there's no alias conflict. And, every shell will have the same alias. Will you please kindly recheck it?
And thanks for giving me a chance, I really appreciate it 🙏

stefanmaric · 2019-12-14T14:05:38Z

Will you please kindly recheck it?

Giving it a look now.

And thanks for giving me a chance, I really appreciate it

Thank you for your help!

stefanmaric

@alvinmatias69 I left a pair of comments, but overall looks good. 👍

stefanmaric · 2019-12-14T14:07:32Z

bin/install

+#
+
+exists() {
+  case "$1" in


@alvinmatias69 here you gotta add a default case. It will say "g alias does exist" in the shells not supported by the alias setup feature.

For non-supported shells, we can simply assume g doesn't exists, and skip the whole thing since we cannot do anything anyway.

Should we skip the alias configuration for all shells or only those shells are not supported?

Not really following.

So the deal here is that, as it stands now, if you try to install g in a shell diff from the ones here (zsh, bash, fish), for example, in ash, you will be promoted to setup an alias in ash, even tho we don't support it (yet, hopefully). And you will also be stuck in a loop of prompts.

This because exit code 0 means "does exist" while exit code 1 means "does not exist".

Since there's no defuault case, when the shell is not supported the default exit code is 0.

So an *) return 1;; at the end would help fix these cases.

Maybe we should invert the exit codes and add explicit returns inside if statements to make it clearer for future readers. 🤔

Or am I missing something? I'm reviewing from my phone so I don't have the big picture. 😄

Oh, I get it. I initially thought that the alias won't be set for all shells if there's an unsupported shell. So, basically return 1 as default so the installer won't recognize it as already exists alias, right? 🤔

That. I think we are on the same page now. 👍

Allrite, I'm already on it

stefanmaric · 2019-12-14T15:11:11Z

bin/install

+
+set_g_alias() {
+  if [ "$G_ALIAS_CHANGED" = true ]; then
+    log_info "info" "configuring $G_ALIAS as alias in $1"


Let's remove this log since it would output "configuring X in file Y" twice for each shell. Since the setup of env vars and the setup of the aliases affect the same files, let's treat them as the same operation.

On some shell (especially zsh), `g` is already used or aliased for other command. Prompt user to enter new alias for `g` on installation. fix [stefanmaric#10](stefanmaric#10)

stefanmaric

@alvinmatias69 ready to merge now. Thanks again for your contribution! ❤️

alvinmatias69 · 2019-12-16T00:22:06Z

Thanks man, really glad to contribute. I'm gonna watch some issues for future improvement 🔥

@alvinmatias69

- Warn users about existing go installations - Improve self-upgrade script - Improve previous installation detection on g-install - Make self-upgrade throw if g was not installed via g-install - Add alias collision detection and setup helper (#11, thanks @alvinmatias69) - Add `download` and `set` commands (#12, thanks @feualpha) * BREAKING: Remove the `--download` option - Add Makefile with lint and format targets for better DX - Fix shellcheck lint errors - Format source code using make format - Improve and update docs to reflect latest contributions

stefanmaric changed the base branch from master to next October 29, 2019 09:39

stefanmaric requested changes Oct 29, 2019

View reviewed changes

alvinmatias69 force-pushed the feature_auto_alias branch from a1f859e to 9894085 Compare October 29, 2019 13:41

alvinmatias69 requested a review from stefanmaric October 29, 2019 13:42

stefanmaric requested changes Nov 7, 2019

View reviewed changes

bin/install Outdated Show resolved Hide resolved

alvinmatias69 force-pushed the feature_auto_alias branch 3 times, most recently from 94e7228 to e98ed14 Compare December 10, 2019 18:18

alvinmatias69 requested a review from stefanmaric December 10, 2019 18:19

stefanmaric requested changes Dec 14, 2019

View reviewed changes

alvinmatias69 force-pushed the feature_auto_alias branch from e98ed14 to 0a79522 Compare December 14, 2019 16:26

feature(install): automatically add alias on conflict command

e0e2fa4

On some shell (especially zsh), `g` is already used or aliased for other command. Prompt user to enter new alias for `g` on installation. fix [stefanmaric#10](stefanmaric#10)

alvinmatias69 force-pushed the feature_auto_alias branch from 0a79522 to e0e2fa4 Compare December 14, 2019 17:41

alvinmatias69 requested a review from stefanmaric December 14, 2019 17:42

stefanmaric approved these changes Dec 15, 2019

View reviewed changes

stefanmaric merged commit c7eba38 into stefanmaric:next Dec 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(install): automatically add alias on conflict command #11

feature(install): automatically add alias on conflict command #11

alvinmatias69 commented Oct 28, 2019

stefanmaric left a comment

stefanmaric left a comment

stefanmaric commented Dec 9, 2019

alvinmatias69 commented Dec 10, 2019

stefanmaric commented Dec 10, 2019

alvinmatias69 commented Dec 10, 2019

stefanmaric commented Dec 14, 2019

stefanmaric left a comment

stefanmaric Dec 14, 2019

alvinmatias69 Dec 14, 2019

stefanmaric Dec 14, 2019

stefanmaric Dec 14, 2019

stefanmaric Dec 14, 2019

alvinmatias69 Dec 14, 2019

stefanmaric Dec 14, 2019

alvinmatias69 Dec 14, 2019

stefanmaric Dec 14, 2019

stefanmaric left a comment

alvinmatias69 commented Dec 16, 2019

feature(install): automatically add alias on conflict command #11

feature(install): automatically add alias on conflict command #11

Conversation

alvinmatias69 commented Oct 28, 2019

stefanmaric left a comment

Choose a reason for hiding this comment

stefanmaric left a comment

Choose a reason for hiding this comment

stefanmaric commented Dec 9, 2019

alvinmatias69 commented Dec 10, 2019

stefanmaric commented Dec 10, 2019

alvinmatias69 commented Dec 10, 2019

stefanmaric commented Dec 14, 2019

stefanmaric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanmaric left a comment

Choose a reason for hiding this comment

alvinmatias69 commented Dec 16, 2019