minicli icon indicating copy to clipboard operation
minicli copied to clipboard

[feat] Add fallback to shell's "read" command.

Open NickSdot opened this issue 2 years ago • 5 comments

I have just been playing with minicli and I like it. The readme says:

Note: If you want to obtain user input, then the readline PHP extension is required as well.

I wondered why this is necessary here. We are in a CLI. So why not leverage the CLI for this functionality instead of fiddling with PHP extensions?

Added

This PR adds functionality to check if the PHP readline extension is available. If not, we fall back on the shell command read, or (tbd) throw and exception if none of the options are available.

Todo / Thoughts

This is just a draft to visualise my idea and see if you are interested. If this is something you want to merge, the following things need to be done or considered:

  1. A match() was used, but I am open to handling this differently. I chose to use the match because it is easy to add more cases. For example, if you are interested in merging this, I would add OS related checks.
  2. Preferably I would throw an exception if none of the match cases has a match. Probably good to wrap this in a try/catch. Open to ideas/feedback.
  3. Both readline and read could return false (or null). As this is not covered in the existing code, I have not touched it yet. Opinions?
  4. Doc blocks aren't ready yet.
  5. Not sure about tests. We are dealing with Infra here.

NickSdot avatar May 17 '23 17:05 NickSdot

The idea is good, but using the extension IMO is a good approach to not add the troubles with diff OS per example. We could add a lot of headaches with edge cases that we don't have with the use of the extension.

This is just my opinion. Let's see what @erikaheidi and @JustSteveKing think about that.

WendellAdriel avatar May 23 '23 11:05 WendellAdriel

As mentioned above I would add this OS cases. It's two more, if I am not mistaken.

I understand your concern @WendellAdriel. Missed edge cases still could happen. But, these are the cases that then could fallback to the extension approach. Imho, it is more beneficial that only fa few 'edge case people' would need to deal with the extension than everyone. No?

NickSdot avatar May 23 '23 14:05 NickSdot

As mentioned above I would add this OS cases. It's two more, if I am not mistaken.

If that's the case and we can have all of them behaving in the same way, it could be a nice addition.

2. Preferably I would throw an exception if none of the match cases has a match. Probably good to wrap this in a try/catch. Open to ideas/feedback.

Yeah, I think that throwing an exception to print a user-friendly message to the terminal would be good

3. Both readline and read could return false (or null). As this is not covered in the existing code, I have not touched it yet. Opinions?

No need, we already use casting with (string) for that.

5. Not sure about tests. We are dealing with Infra here.

You should add test cases if possible to check in diff OS (while skipping others when running on diff OS) in this test case: https://github.com/minicli/minicli/blob/main/tests/Feature/InputTest.php

WendellAdriel avatar May 23 '23 16:05 WendellAdriel

I like this idea! I don't see why not to have this, if we can make sure it has tests.

erikaheidi avatar Jun 01 '23 18:06 erikaheidi

Alright! Just waited for a second 'go' from y'all. I'll get on it soon-ish.

NickSdot avatar Jun 02 '23 13:06 NickSdot

Ended up not using this. Y'all know how it is. Will not send a PR. Sorry for over advertising. 😅

NickSdot avatar Jun 24 '24 03:06 NickSdot