rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
381 stars 20 forks source link

Dispatch User Prompts to Dialog Tool #23

Closed gabyx closed 5 years ago

gabyx commented 5 years ago

Introduce a small tool "dialog" which dispatches all dialog prompts in base-template.sh to a script. An example implementation with python3.6 and higher is given in examples/tools/dialog.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 195


Changes Missing Coverage Covered Lines Changed/Added Lines %
cli.sh 26 28 92.86%
base-template.sh 15 23 65.22%
<!-- Total: 41 51 80.39% -->
Totals Coverage Status
Change from base Build 193: -0.3%
Covered Lines: 1543
Relevant Lines: 1689

💛 - Coveralls
gabyx commented 5 years ago

Works nicely: image

Also works from any GUI :-) Tested with VS Code.

rycus86 commented 5 years ago

I'm a bit concerned about trying to detect if we're running from a connected terminal in a cross-platform way. Can't we just check if we have a dialog tool configured, and if we do use that otherwise just try reading from the terminal as before?

gabyx commented 5 years ago

I'm a bit concerned about trying to detect if we're running from a connected terminal in a cross-platform way. Can't we just check if we have a dialog tool configured, and if we do use that otherwise just try reading from the terminal as before?

I think it should work, shouldn it?. The thing, is the dialog tool should really only be the case when running without a terminal attached. Because when running in the terminal you can just answer there instead of the GUI. Or should we do it like you said. and I can provide for my solution code in the run script to detect if we have a terminal attached. So far the provided solution works. At least on Windows with Git-Bash (from the official git install) etc. Should also work in Linux.

gabyx commented 5 years ago

At least I tested it on MacOs as well.

rycus86 commented 5 years ago

OK, I think this looks good to me, I'd just like to start with keeping everything as-is, except if you configure a dialog tool, in which case that is invoked instead of the read -r. Could you change it like that and squash please? Thanks!

gabyx commented 5 years ago

Ok, cool!

gabyx commented 5 years ago

Force pushed.

rycus86 commented 5 years ago

This looks good, just needs a rebase/merge for the conflicts and a squash after perhaps. There's a few typos here and there, but I can take care of them after merging this PR.

gabyx commented 5 years ago

Maybe we could also show the stderr of the runned hook over the dialog tool, maybe, maybe in another PR :-)

rycus86 commented 5 years ago

Sorry, had to revert this one, was breaking all the user prompts without the dialog tool. :( I think the problem was here:

    else
        # Read from stdin (because its connected)
        # shellcheck disable=SC2059
        printf "$TEXT $HINT_TEXT [$SHORT_OPTIONS]: "
        read -r VARIABLE
    fi

This looks like we were loading user input into $VARIABLE instead of the actual variable name. I think read -r $VARIABLE could work though. Would you like to reopen this and give it another try? I think where this slipped through, is that we replaced the whole call to show_prompt in the test setup phase with the default answer. Instead of this, we should perhaps just change call_script "$DIALOG_TOOL" to return what we want it to return.

rycus86 commented 5 years ago

@gabyx :arrow_up: in case you haven't seen

gabyx commented 5 years ago

Oh, I think you are right.

gabyx commented 5 years ago

I reopen this, :-)

gabyx commented 5 years ago

See #28