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: get completion for composer commands #5756

Merged

Conversation

GuySartorelli
Copy link
Collaborator

@GuySartorelli GuySartorelli commented Jan 31, 2024

Figured I'd get this done while autocompletion is on my brain.

The Issue

There's no autocompletion for composer commands right now, other than what cobra can give us.

How This PR Solves The Issue

If the cwd is inside a project with a running web container, we request completions from composer in the container.

This should work for all shells cobra supports autocompletion for - which is more than composer itself!

I've also added autocompletion for the create command (pretty much only useful for flags, but that's not nothing) and hidden the create-project command from completion suggestions.

Manual Testing Instructions

Running in any shell cobra supports for autocompletion, type ddev composer <tab>, and other combinations you'd expect to get autocompletions for (like ddev composer re<tab> to get completion suggestions for composer remove and composer require)

Also try out ddev composer create --<tab> to see completion suggestions for the create command.

Automated Testing Overview

Added a couple of small tests to check we do in fact get expected completions from composer. That test is pretty light because we don't need to actually test composer's auto completion suggestions - we just need to validate that we're passing the completion request through, and getting a correct response.

Related Issue Link(s)

N/A

Release/Deployment Notes

Should just work out of the box, no breaking changes, just nice auto complete goodness.

@GuySartorelli GuySartorelli requested a review from a team as a code owner January 31, 2024 10:03
Copy link

github-actions bot commented Jan 31, 2024

@rfay
Copy link
Member

rfay commented Jan 31, 2024

Although I'm a bash stalwart, there are an awful lot of zsh users out there, and it seems like most of them have oh-my-zsh.

@GuySartorelli
Copy link
Collaborator Author

I use ohmyzsh myself. Composer doesn't currently support autocompletion for zsh. There's a ohmyzsh plugin that provides composer completion - I'd say if zsh stans want completion for ddev composer before composer itself is resdy to provide them, a similar plugin probably needs to be created which is clearly outside the scope of this pr.

@GuySartorelli
Copy link
Collaborator Author

GuySartorelli commented Feb 1, 2024

Upon reflection it occurred to me that cobra will handle making the completion results correct for a given shell - and indeed it does. So we can just tell composer we're using bash (just to get the completion suggestions) and then feed those to cobra, and cobra can feed them to our shell in the correct format.

Testing locally in zsh it works well. I don't have fish so I can't check that - but there's no reason to assume it won't work.

I'll update the PR description to match.

@GuySartorelli
Copy link
Collaborator Author

Note that I've made the adjustment as a separate commit so that this PR still contains a record of what the code looked like for finding the host shell. That might still come in handy some day.

@GuySartorelli
Copy link
Collaborator Author

And had one last epiphany. I said there'd be no container when running ddev composer create but that's just ridiculous - how would it run if there's no container? So I've added completion for that command now as well.

Note

I've also hidden ddev composer create-project so we're not including it in completion results - no need to encourage people to use it.
It'll still tell people to use create instead if people run it.
If you want this added back to the completion results let me know and I'll unhide it - but it really doesn't seem like it should be in completion suggestions.

Assuming CI goes green, this should be good to go now.

Comment on lines -26 to +25
FParseErrWhitelist: cobra.FParseErrWhitelist{
UnknownFlags: true,
},
Short: "Executes 'composer create-project' within the web container with the arguments and flags provided",
DisableFlagParsing: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm 90% sure DisableFlagParsing and FParseErrWhitelist: cobra.FParseErrWhitelist{UnknownFlags: true} are functionally equivalent as far as running the command goes - but DisableFlagParsing is explicitly required for the completion function to be called. Not sure why, couldn't find any docs on it, but that's why I've made this particular change.

pkg/exec/exec.go Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changes in this file were automatically made by my IDE because in my first commit I had added some code there, then in the second commit I removed it again.
See #5756 (comment)

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

This seems great to me, manually tested and it did all the things. It can go in when the tests are green; currently TestComposerCreateAutocomplete fails.

@GuySartorelli
Copy link
Collaborator Author

GuySartorelli commented Feb 5, 2024

Running make test TESTARGS="-run TestComposerCreateAutocomplete" I'm not able to reproduce the failure.
I don't know enough about the workflow setup to know what I could change locally to match the CI environment, which is normally what my next step would be here.

Could you please check if you can reproduce this locally, or give me some pointers about either how to reproduce it or what might be causing it to fail?

@rfay
Copy link
Member

rfay commented Feb 5, 2024

It succeeds for me as well. I ran the whole file's tests in Goland, with GOTEST_SHORT=12 (drupal10). The buildkite tests use GOTEST_SHORT=8 (drupal9)

But every single test failed, so the assumption has to be crosstalk between tests, some state is not being cleaned up.

Try make test TESTARGS="-run TestComposer" to get all composer-related tests.

Or try running all the tests in the cmd directory with GOTEST_SHORT=12 make testcmd

BTW, if you're not doing this already, you'll want to be running tests in vscode, it does a great job of running individual tests, and of course you can debug through them.

You're awesome!

Could you please PM me with your contact info (any way you want, Discord might be easiest?) I'd like to set up a meeting soon.

@rfay
Copy link
Member

rfay commented Feb 5, 2024

make testcmd demonstrates it, https://gist.github.com/rfay/967350ffd1038cb2f2db7902f2cdf1b5

The tests run in this order result in the failure:

=== RUN   TestCmdVersion
=== RUN   TestCustomCommands
=== RUN   TestLaunchCommand
=== RUN   TestMysqlCommand
=== RUN   TestPsqlCommand
=== RUN   TestNpmYarnCommands
=== RUN   TestComposerCreateCmd
=== RUN   TestComposerCreateAutocomplete
=== RUN   TestComposerCmdCreateConfigInstall

@rfay
Copy link
Member

rfay commented Feb 5, 2024

Oh look, isn't this the same thing happening here? https://github.com/ddev/ddev/actions/runs/7778487300/job/21208251886?pr=5058

That's in #5058 which was recently rebased to upstream/master.

However, upstream/master doesn't seem to be seeing this.

@GuySartorelli
Copy link
Collaborator Author

Try make test TESTARGS="-run TestComposer" to get all composer-related tests.
Or try running all the tests in the cmd directory with GOTEST_SHORT=12 make testcmd

Thanks! That worked, and I've adjusted the test accordingly. I was taking advantage of the site to test being started before my test run, but it seems that in some cases other tests will stop a site - so I need to make sure the sites are started before the test code is run. I've done this to both of the new autocompletion tests.

BTW, if you're not doing this already, you'll want to be running tests in vscode, it does a great job of running individual tests, and of course you can debug through them.

Thanks! I find with my workflow running tests in the terminal is preferable, though if I ever get a test that I need to debug properly I'll probably do that via vscode.

You're awesome!

Aww, shucks, thanks.

Could you please PM me with your contact info (any way you want, Discord might be easiest?) I'd like to set up a meeting soon.

I've flicked you an email.

@rfay
Copy link
Member

rfay commented Feb 6, 2024

Congratulations!

@GuySartorelli
Copy link
Collaborator Author

Looks like this is passing (just colima to go but with everything else passing I'd be shocked if that had a legitimate failure)

@rfay rfay merged commit edb09ef into ddev:master Feb 6, 2024
18 checks passed
@rpkoller
Copy link
Collaborator

rpkoller commented Feb 6, 2024

i've updated to the latest version of HEAD-edb09ef . but somehow i am unable to utilize the autocomplete. with the example from the manual testing instructions i get
Screenshot 2024-02-06 at 16 38 04
for ddev composer <tab> and
Screenshot 2024-02-06 at 16 38 25
actually no options at all to pick from for ddev composer create --. i am on macos sonoma 14.3 with zsh 5.9 (x86_64-apple-darwin23.0) out of the box in the terminal.

@rfay
Copy link
Member

rfay commented Feb 6, 2024

Is autocomplete in general working for you @rpkoller ? For example, does ddev l<tab> get you the things it should? I've been having funky problems with bash autocomplete in general. Are you using bash or zsh?

@rpkoller
Copy link
Collaborator

rpkoller commented Feb 6, 2024

ddev l<tab> has no effect at all (*disclaimer but i haven't installed any autocomplete related with homebrew - i just tested because the issue summary stated it would work out of the box with nothing else required). And I am using the default for sonoma 14.3, zsh 5.9 (x86_64-apple-darwin23.0)

@rfay
Copy link
Member

rfay commented Feb 6, 2024

You have to install autocompletion or any of this to work. https://ddev.readthedocs.io/en/latest/users/install/shell-completion/

@GuySartorelli GuySartorelli deleted the 20240131_GuySartorelli_composer-completion branch February 6, 2024 20:04
@rpkoller
Copy link
Collaborator

rpkoller commented Feb 8, 2024

I use ohmyzsh myself. Composer doesn't currently support autocompletion for zsh. There's a ohmyzsh plugin that provides composer completion - I'd say if zsh stans want completion for ddev composer before composer itself is resdy to provide them, a similar plugin probably needs to be created which is clearly outside the scope of this pr.

on brief addition I did some digging trying to understand zsh completions and how to get them working for me (i've opened an issue about that #5803) and i've noticed that there actually is a completion for composer already available at /usr/share/zsh/5.9/functions/_composer (with Sonoma) . So in consequence autocompletion should work for composer commands if i understand things correctly?

@GuySartorelli
Copy link
Collaborator Author

i've noticed that there actually is a completion for composer already available at /usr/share/zsh/5.9/functions/_composer (with Sonoma) . So in consequence autocompletion should work for composer commands if i understand things correctly?

To the best of my knowledge, that will work for composer itself (i.e. composer <tab>) but not for the composer command in ddev (i.e. ddev composer <tab>)

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

Successfully merging this pull request may close these issues.

None yet

3 participants