Skip to content

Conversation

inkarkat
Copy link
Member

@inkarkat inkarkat commented Aug 6, 2021

Supplying the user confirmation via "yes".

Before submitting a pull request, please make sure the following is done:

  • Fork the repository and create your branch from master.
  • If you've added code that should be tested, add tests!
  • Ensure the test suite passes.
  • Format your code with ShellCheck.
  • Include a human-readable description of what the pull request is trying to accomplish.
  • Steps for the reviewer(s) on how they can manually QA the changes.
  • Have a fixes #XX reference to the issue that this pull request fixes.

This is an extended alternative implementation of #152 with added refactoring and test coverage.

Supplying the user confirmation via "yes".
The user confirmation query had been duplicated (once) in the code.
By just reading a single character (y for yes, anything else: no).

Closes todotxt#152
By switching from "yes" (that endlessly prints newline-separated "y"s) to "printf y".
Negative means "anything but y", so "n", "x", and Enter all apply.
So that the user's typed answer is not recorded directly after it, but with separation: "Foo? (y/n) y" instead of "Foo? (y/n)y".
@inkarkat
Copy link
Member Author

inkarkat commented Aug 6, 2021

Ah, bummer. That's what I meant when I commented It would be nice if someone could verify that this also works on MacOS; that one's causing compatibility problems most often.

The Bash on MacOS (looks like it's Bash 3.2.57(1)-release), doesn't support read -N yet :-( This was only added in Bash 4.1.

Mac OS still ships with Bash 3.2 :-( Fall back to the original prompting that requires conclusion via Enter then.
Note: Even though the tests use "printf y", this still gets accepted, as there'll be EOF after that. In real use (when stdin from the terminal stays open), a concluding Enter is mandatory, though.
@karbassi
Copy link
Member

karbassi commented Aug 6, 2021

The Bash on MacOS (looks like it's Bash 3.2.57(1)-release), doesn't support read -N yet :-( This was only added in Bash 4.1.

Welcome to macOS land! I can't speak for others, but first thing I do is upgrade the default apps (bash, zsh, gnu*, etc)

@karbassi karbassi self-requested a review August 6, 2021 20:04
Copy link
Member

@karbassi karbassi left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@karbassi karbassi merged commit ee94a3f into todotxt:master Aug 6, 2021
@inkarkat inkarkat deleted the direct-prompt branch August 7, 2021 06:29
wwalker pushed a commit to wwalker/todo.txt-cli that referenced this pull request Sep 19, 2021
* Tests: Add coverage for del / move without -f, but with prompting

Supplying the user confirmation via "yes".

* Cosmetics: Align inconsistent spacing for before (y/n) prompt

* Refactoring: Extract confirm() function

The user confirmation query had been duplicated (once) in the code.

* Refactoring: confirm(): Leave early if forced

* Return from user prompt without requiring Enter

By just reading a single character (y for yes, anything else: no).



* Tests: Ensure that only a single "y" concludes the confirmation

By switching from "yes" (that endlessly prints newline-separated "y"s) to "printf y".

* t1800-del: Add coverage for negative confirmation

Negative means "anything but y", so "n", "x", and Enter all apply.

* Cosmetics: Add trailing space after (y/n) prompt

So that the user's typed answer is not recorded directly after it, but with separation: "Foo? (y/n) y" instead of "Foo? (y/n)y".

*Compatibility: "read -N 1" is only available in Bash 4.1+

Mac OS still ships with Bash 3.2 :-( Fall back to the original prompting that requires conclusion via Enter then.
Note: Even though the tests use "printf y", this still gets accepted, as there'll be EOF after that. In real use (when stdin from the terminal stays open), a concluding Enter is mandatory, though.

Closes todotxt#152
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