vermiculus / magithub

**DEPRECATED - please use Forge instead!** -- Magit-based interfaces to GitHub
GNU General Public License v3.0
579 stars 63 forks source link

magithub-clone should use `yes-or-no-p` #306

Open Silex opened 6 years ago

Silex commented 6 years ago

That way this part of my config would work as expected:

;; make all "yes or no" prompts show "y or n" instead
(fset 'yes-or-no-p 'y-or-n-p)

That is, I don't want to have to type "yes" or "no" in full.

Also, I think in overall the C-u* option in these prompts are not really usefull. If you want to follow the magit conventions, do these configurations in some popup like the branch popup in magit, which allows you to configure upstream & pushremote, etc. You could have some "settings" popup.

Also when the question is "yes or no" I expect yes-or-no-p, not to be able to configure the default behavior. If I want to configure the default answer, I expect to find the instructions in the docstring for magit-clone, or some emacs variable with configure-variable, or some configuration popup.

vermiculus commented 6 years ago

I'm not convinced this is desirable, but I'd be open to try out a pull request. Personally, I find the decision for 'do I want to answer this again' to be most relevant/obvious when I'm answering the question.

A configuration popup for this would be huge and would only grow with time, so I'm not particularly keen on doing that (but I'm open to the option).

Silex commented 6 years ago

I'm not convinced this is desirable, but I'd be open to try out a pull request. Personally, I find the decision for 'do I want to answer this again' to be most relevant/obvious when I'm answering the question.

I'm not saying get rid of the feature, I'm saying favor user customization through defcustom. You could replace your questions with:

(yes-or-no-p "Do you want to clone bla to foo? (see `magithub-clone-policy`)"

And magithub-clone-policy would be a choice of unconfigured, ask, always-yes, with unconfigured adding the parenthesis help in the question above.

Anyway, I realised I'm picky about this because I come from https://github.com/dgtized/github-clone.el and expect the same behavior. You obviously do it the way you think is the best for your package, but FWIW I think the confirmation question is a bit useless (because we called M-x magithub-clone so we want to clone), and that you'd select the pwd that the clone will be done into, not the full path. I understand your way allows you to clone in a directory that is named differently (e.g you can clone magit/magit in /tmp/test), but in my experience you rarely want this and having to type the full path is kinda annoying and error-prone (which is probably why you added the confirmation question). If you enter the wrong pwd all you need to do is delete the subdirectory so errors are not harmful, and if you really want another directory name it's quick to rename after the clone.

wyuenho commented 6 years ago

@vermiculus I have (fset 'yes-or-no-p 'y-or-n-p) set as well, magithub is the only package out of about 200 that I'm using + the builtins that doesn't respect me. Short of overcomplicating this with popups and whatnot, can you just use the standard yes-or-no-p and y-or-n-p?

wyuenho commented 6 years ago

If you need a "default" answer, just ask again, like "Do you want to answer {last answer} for the rest of the questions? (Y/n)". This is how pretty much every CLI program does it. There's no need to introduce some universal argument weirdness.

vermiculus commented 6 years ago

@wyuenho The trouble comes when users disagree on what is dangerous vs. what is not. Some things I see as dangerous and others do not (submitting a comment); some things others tend to agree (submit a pull request).

Silex commented 6 years ago

So, if we make a PR you'd be willing to merge it?

wyuenho commented 6 years ago

The trouble comes when users disagree on what is dangerous vs. what is not.

That's why you ask whether the users wants to answer Y for all. That's the simplest solution, a popup would be ideal, but that's a nice to have. I just don't want to have to deal with Emacs's stupid yes-or-no/y-no-n discrepancy.

vermiculus commented 6 years ago

@wyuenho 'Y for all' is not appropriate though; I do not want to remove all yes-or-no-p prompts. There are some actions that are dangerous that I do not want to fat-finger.

@Silex It depends on the change, but if it still solves the problem of customization, I'd at least consider it.

Silex commented 6 years ago

@vermiculus: I think an alternative @wyuenho hinted at is instead of having:

Do you want to clone vermiculus/magithub to work/magithub? (yes, no, C-u*)

Have it in two questions:

Do you want to clone vermiculus/magithub to work/magithub? (y/n)
Do you want this action to be the default? (y/n)

Personnally I still prefer this option:

(yes-or-no-p "Do you want to clone bla to foo? (configure `magithub-confirm-clone` to avoid this question)"

Maybe with some wizard to configure all the policies at once? The first time magithub is installed, if nothing is configured ask a question like Looks like magithub is not configured, do you want to configure the default actions now? and then it simply opens customize-group magithub-default-actions, which ends up configuring (setq magithub-confirm-clone nil), etc.