Respect autocomplete setting when parsing subcommands

davetron5000 / gli

Make awesome command-line applications the easy way

http://davetron5000.github.io/gli

Apache License 2.0

1.26k stars 102 forks source link

Respect autocomplete setting when parsing subcommands #288

Closed atareshawty closed 5 years ago

atareshawty commented 5 years ago

In response to #287

The first commit adds tests to demonstrate what (I think) the expected behavior should be:

In loose mode with autocomplete = false, the parser doesn't attempt to autocomplete the sub command
- Instead, the partial subcommand ends up as an argument to the parent command
In strict mode with autocomplete = true, the parser raises an error with the message: error: Too many arguments for command

I'm starting the PR as a draft because I'm not 100% sure if I'm correctly stating the expected behavior.

The next commit fixes the issue and makes the tests pass 🚀

davetron5000 commented 5 years ago

Nice catch on the strict arg handling. Your overall approach looks good.

atareshawty commented 5 years ago

Thanks! This is ready for review then 😄

davetron5000 commented 5 years ago

OK, cool. A couple of questions just to make sure I'm following it:

Looks like the bug was that the autocomplete configuration was not properly being passed down when recursively finding commands, so the system defaulted to the wrong thing?
Did you check this against your app to make sure it does work? The integration testing in this repo isn't very good or I'd have encouraged you to write one, but I don't think it's necessary since you ahve a good unit test and you checked it locally

atareshawty commented 5 years ago

Looks like the bug was that the autocomplete configuration was not properly being passed down when recursively finding commands, so the system defaulted to the wrong thing?

Correct.

Did you check this against your app to make sure it does work?

I just checked again and it does fix the issue. I'll post a branch in the reproduction repo I linked in #287

Edit: I clicked the button before typing my answer to your second question😅

atareshawty commented 5 years ago

Here is an example when using the patched version: https://github.com/Root-App/gli-auto-complete/tree/verify-fix. I've updated the README to include how the output changes with the patch

davetron5000 commented 5 years ago

OK, awesome. I will merge and release here in a few…

davetron5000 commented 5 years ago

Released just now as 2.18.1

Thanks again for the fix!

atareshawty commented 5 years ago

No problem! Thank you for maintaining this great gem