Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

NickSdot
Copy link
Contributor

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.

@WendellAdriel
Copy link
Member

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.

@NickSdot
Copy link
Contributor Author

NickSdot commented May 23, 2023

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?

@WendellAdriel
Copy link
Member

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

@erikaheidi
Copy link
Member

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

@NickSdot
Copy link
Contributor Author

NickSdot commented Jun 2, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants