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

Single word is not completed in PowerShell #1847

Closed
totkeks opened this issue Nov 7, 2022 · 8 comments · Fixed by #1850
Closed

Single word is not completed in PowerShell #1847

totkeks opened this issue Nov 7, 2022 · 8 comments · Fixed by #1850

Comments

@totkeks
Copy link
Contributor

totkeks commented Nov 7, 2022

When I use the Powershell completion in oh-my-posh (JanDeDobbeleer/oh-my-posh#3050), it fails to complete when I want to complete the first word, e.g. con<tab> or com<tab> - should result in config or completion respectively. Instead neither happens.

After a little digging I found that OMP uses this project and that the PowerShell completion might have a bug.

$Values = $Out | ForEach-Object {
		#Split the output in name and description
		$Name, $Description = $_.Split("`t", 2)
		__oh-my-posh_debug "Name: $Name Description: $Description"

		# Look for the longest completion so that we can format things nicely
		if ($Longest -lt $Name.Length) {
			$Longest = $Name.Length
		}

		# Set the description to a one space string if there is none set.
		# This is needed because the CompletionResult does not accept an empty string as argument
		if (-Not $Description) {
			$Description = " "
		}
		@{Name = "$Name"; Description = "$Description" }
	}

When there is only one completion result, which is oddly the case for oh-my-posh __complete con but not for oh-my-posh __complete config ex, then the above code makes $Values a hashtable and not an array, which results in a hidden error down the line, because a hashtable has no Length property.

if (($Directive -band $ShellCompDirectiveNoFileComp) -ne 0 ) {
		__oh-my-posh_debug "ShellCompDirectiveNoFileComp is called";

		if ($Values.Length -eq 0) {
			# Just print an empty string here so the
			# shell does not start to complete paths.
			# We cannot use CompletionResult here because
			# it does not accept an empty string as argument.
			""
			return
		}
	}

My simple workaround has been forcing $Values to become an array by doing this [Array]$Values in the first code fragment.

@marckhouzam
Copy link
Collaborator

Thanks for the report @totkeks.
At the moment I have tested using powershell on a Mac and I cannot reproduce the problem using helm comp<TAB>.

My powershell version is PowerShell 7.2.4.
Are you seeing the problem on a Windows machine?
What version of Powershell do you use?

@totkeks
Copy link
Contributor Author

totkeks commented Nov 8, 2022

I'm using 7.2.7 on Windows. It might be that helm produces more than one suggestion with the __complete command that is internally called.

For example oh-my-posh delivers this output for just conf

> oh-my-posh __complete conf
config  Interact with the config
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

And this for config ex

> oh-my-posh __complete config ex
export  Export your config
export
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

It might be that your helm command returns more than one result for the completion you requested in your test. The issue only appears if there is only a single suggestion returned by the __complete call.

@marckhouzam
Copy link
Collaborator

I installed oh-my-posh on my Mac and upgraded to powershell 7.2.7 and I still can't reproduce.

For example oh-my-posh delivers this output for just conf

> oh-my-posh __complete conf
config  Interact with the config
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

This should be fine. I see the same thing.

And this for config ex

> oh-my-posh __complete config ex
export  Export your config
export
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

This is also fine, although it is actually bizarre that export appears twice. I looked at the code and I believe it is because export is a sub-command but also listed as a validArg here: https://github.com/JanDeDobbeleer/oh-my-posh/blob/0ce34eef12d5b4f4fc90ad32dfeec2209e093e4a/src/cli/config.go#L18-L22

But that is not causing a problem anyway.

Maybe the hash vs array only happens on windows. I'll see if I can test on Windows.

Just to be clear, this is how I test things:

$ pwsh
PowerShell 7.2.7
Copyright (c) Microsoft Corporation.

https://aka.ms/powershell
Type 'help' to get help.

PS /tmp> ./oh-my-posh completion powershell | Out-String | Invoke-Expression
PS /tmp> ./oh-my-posh conf<tab>
PS /tmp> ./oh-my-posh config

@totkeks
Copy link
Contributor Author

totkeks commented Nov 8, 2022

Thanks for the update. Yes, your code is correct, that's how I do it, except the last line, which it doesn't complete to.

Another option to test this, also on your Mac, would be to see if they use a different way to handle ForEach-Object:

> (@(1,2) |foreach-object { @{Name = $_ } }).GetType().Name
Object[]

> (@(1) | foreach-object { @{Name = $_ } }).GetType().Name
Hashtable

The first line should return Object[] and the second line should return Hashtable.

@marckhouzam
Copy link
Collaborator

> (@(1,2) |foreach-object { @{Name = $_ } }).GetType().Name
Object[]

> (@(1) | foreach-object { @{Name = $_ } }).GetType().Name
Hashtable

Same result for me.
I wonder if it could be something to do with descriptions.
Could you turn off descriptions and see if it changes anything? To turn off descriptions instead of __complete use __completeNoDesc. So you can do this:

oh-my-posh completion powershell | sed s,__complete,__completeNoDesc,g | Out-String | Invoke-Expression
oh-my-posh com<tab>

@totkeks
Copy link
Contributor Author

totkeks commented Nov 11, 2022

It didn't change anything. But I figured out why it doesn't work for me but works for you. In my PowerShell profile I have Set-StrictMode -Version Latest.
https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/set-strictmode?view=powershell-7.2

When Set-StrictMode is off, PowerShell has the following behaviors:
[...]
* References to non-existent properties return $Null

With strict mode on, the opposite happens and since Hashtable has no Length property, an exception is thrown.

According to this old, archived post, they added a feature in PowerShell V3, that lets Length return the same value as Count.
https://web.archive.org/web/20150612170424/http://blogs.msdn.com/b/powershell/archive/2012/06/14/new-v3-language-features.aspx
Which is why the comparison $Values.Length -eq 0 works for you.

So, I guess I'm the only person that has strict mode enabled. 😅

Would you mind changing the template code to this, for the sake of correctness?

[Array]$Values = $Out | ForEach-Object {

@marckhouzam
Copy link
Collaborator

Thanks for figuring this out!
Do you want to post a PR?

@totkeks
Copy link
Contributor Author

totkeks commented Nov 11, 2022

Yes, I want to do that. I haven't done this before but would like to as a learning experience. I will get to it in the next few days.

totkeks added a commit to totkeks/cobra that referenced this issue Nov 13, 2022
Dav-14 added a commit to formancehq/cobra that referenced this issue Mar 15, 2024
* Create unit test illustrating unknown flag bug (spf13#1854)

Created a unit test that tests the unknown flag
error message when the unknown flag is located
in different arg positions.

* Update stale.yml (spf13#1863)

* fix: force ForEach-Object to return array in pwsh completion (spf13#1850)

Fixes spf13#1847

* Makefile: add target richtest (spf13#1865)

Don't require contributors to install richgo but keep it as an option and for CI

* build(deps): bump golangci/golangci-lint-action from 3.2.0 to 3.3.1 (spf13#1851)

Bumps [golangci/golangci-lint-action](https://github.com/golangci/golangci-lint-action) from 3.2.0 to 3.3.1.
- [Release notes](https://github.com/golangci/golangci-lint-action/releases)
- [Commits](golangci/golangci-lint-action@v3.2.0...v3.3.1)

---
updated-dependencies:
- dependency-name: golangci/golangci-lint-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update kubescape org (spf13#1874)

Signed-off-by: David Wertenteil <dwertent@armosec.io>

* ci: deprecate go 1.15 (spf13#1866)

Remove testing for go 1.15 to allow CI to pass, but don't force projects to upgrade.

* fix: conflict import name with variable (spf13#1879)

`template` is an import in `cobra.go` file and also used as a variable
name, which masks the library in the scope of that function.

* Update badge route (spf13#1884)

Based on
badges/shields#8671

* fix: func name in doc strings (spf13#1885)

Corrected the function name at the start of doc strings, as per the convention
outlined in official go documentation: https://go.dev/blog/godoc

* completions: do not detect arguments with dash as 2nd char as flag (spf13#1817)

Fixes spf13#1816

Previously, arguments with a dash as the second character (e.g., 1-ff00:0:1)
were detected as a flag by mistake. This resulted in auto completion misbehaving
if such an argument was last in the argument list during invocation.

* build(deps): bump github.com/inconshreveable/mousetrap (spf13#1872)

* Add documentation about disabling completion descriptions (spf13#1901)

* Improve MarkFlagsMutuallyExclusive example in User Guide (spf13#1904)

* Update shell_completions.md (spf13#1907)

align documentation with the code : completions.go:452

* build(deps): bump golangci/golangci-lint-action from 3.3.1 to 3.4.0 (spf13#1902)

* Removes stale bot from GitHub action (spf13#1908)

Signed-off-by: John McBride <jpmmcbride@gmail.com>

* Add keeporder to shell completion (spf13#1903)

This allows programs to request the shell to maintain the order of completions that was returned by the program

* Add support for PowerShell 7.2+ (spf13#1916)

PowerShell 7.2 has changed the way arguments are passed to executables.
This was originally an experimental feature in 7.2, but as of 7.3 it is
built-in. A simple "" is now sufficient for passing empty arguments, no
back-tick escaping is required.

Fixes spf13#1849

Signed-off-by: Oldřich Jedlička <oldium.pro@gmail.com>
Co-authored-by: Oldřich Jedlička <oldrich.jedlicka@rohlik.cz>

* ci: deprecate go 1.16 (spf13#1926)

* ci: test Golang 1.20 (spf13#1925)

* update copyright year (spf13#1927)

* Update projects_using_cobra.md (spf13#1932)

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* Document suggested layout for subcommands (spf13#1930)

Signed-off-by: Luiz Carvalho <lucarval@redhat.com>

* Allow sourcing zsh completion script (spf13#1917)

Although it is not the recommended approach, sourcing a completion
script is the simplest way to get people to try using shell completion.
Not allowing it for zsh has turned out to complicate shell completion
adoption.  Further, many tools modify the zsh script to allow sourcing.

This commit allows sourcing of the zsh completion script.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

* Update main image to better handle dark background (spf13#1883)

Fixes spf13#1880

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
Co-authored-by: Deleplace <deleplace2015@gmail.com>

* Fix typo in fish completions (spf13#1945)

* build(deps): bump golangci/golangci-lint-action from 3.4.0 to 3.5.0 (spf13#1971)

* Fix grammar: 'allows to' (spf13#1978)

The use in generated bash completion files is getting flagged by
Lintian (the Debian package linting tool).

Signed-off-by: Taavi Väänänen <hi@taavi.wtf>

* test: make fish_completions_test more robust (spf13#1980)

Use temporary files instead of assuming the current directory is
writable. Also, if creating a temporary file still returns an error,
prevent the test from failing silently by replacing `log.Fatal` with
`t.Fatal`.

* powershell: escape variable with curly brackets (spf13#1960)

This fixes an issue with program names that include a dot, in our case
`podman.exe`. This was caused by the change in commit 6ba7ebb.

Fixes spf13#1853

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

* build(deps): bump golangci/golangci-lint-action from 3.5.0 to 3.6.0 (spf13#1976)

* Move documentation sources to site/content (spf13#1428)

* Add 'one required flag' group (spf13#1952)

* golangci: enable 'unused' and disable deprecated replaced by it (spf13#1983)

* doc: fix typo, Deperecated -> Deprecated (spf13#2000)

* minor corrections to unit tests (spf13#2003)

* build(deps): bump golangci/golangci-lint-action from 3.6.0 to 3.7.0 (spf13#2021)

* command: temporarily disable G602 due to securego/gosec#1005 (spf13#2022)

* ci: test golang 1.21 (spf13#2024)

* Customizable error message prefix (spf13#2023)

* feat: add getters for flag completions (spf13#1943)

* Add notes to doc on preRun and postRun condition (spf13#2041)

* build(deps): bump actions/setup-go from 3 to 4 (spf13#1934)

* build(deps): bump github.com/cpuguy83/go-md2man/v2 from 2.0.2 to 2.0.3 (spf13#2047)

* Allow running persistent run hooks of all parents (spf13#2044)

Currently, only one of the persistent pre-runs and post-runs is executed.
It is always the first one found in the parents chain, starting at this command.
Expected behavior is to execute all parents' persistent pre-runs and post-runs.

Dependent projects implemented various workarounds for this:
- manually building persistent hook chains (in every hook).
- applying some kind of monkey-patching on top of Cobra.

This change eliminates the necessity for such workarounds
by allowing to set a global variable EnableTraverseRunHooks.

Tickets:
- spf13#216
- spf13#252

Signed-off-by: Volodymyr Khoroz <volodymyr.khoroz@foundries.io>

* Fix linter errors (spf13#2052)

When using golangci-lint v1.55.0 some new errors were being reported.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

* Don't complete --help flag when flag parsing disabled (spf13#2061)

Fixes spf13#2060

When a command sets `DisableFlagParsing = true` it requests the
responsibility of doing all the flag parsing. Therefore even the
`--help/-f/--version/-v` flags should not be automatically completed
by Cobra in such a case.

Without this change the `--help/-h/--version/-v` flags can end up being
completed twice for plugins: one time from cobra and one time from the
plugin (which has set `DisableFlagParsing = true`).

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

* Add tests for flag completion registration (spf13#2053)

Different problems have been reported about flag completion registration.
These two tests are the cases that were not being verified but had been
mentioned as problematic.

Ref:
- spf13#1320
- spf13#1438 (comment)

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

* Replace all non-alphanumerics in active help env var program prefix (spf13#1940)

* Replace all non-alphanumerics in active help env var program prefix

There are other characters besides the dash that are fine in program
names, but are problematic in environment variable names. These include
(but are not limited to) period, space, and non-ASCII letters.

* Another change in docs to mention non-ASCII-alphanumeric instead of just dash

* build(deps): bump actions/checkout from 3 to 4 (spf13#2028)

* Support usage as plugin for tools like kubectl (spf13#2018)

In this case the executable is `kubectl-plugin`, but we run it as:

    kubectl plugin

And the help text should reflect the actual usage of the command.

To create a plugin, add the cobra.CommandDisplayNameAnnotation:

    rootCmd := &cobra.Command{
        Use: "plugin",
        Annotations: map[string]string{
            cobra.CommandDisplayNameAnnotation: "kubectl plugin",
        }
    }

Internally this change modifies CommandPath() for the root command to
return the command display name instead of the command name. This is
used for error messages, help text generation, and completions.

CommandPath() is expected to have spaces and code using it already
handle spaces (e.g replacing with _), so hopefully this does not break
anything.

Fixes: spf13#2017

Signed-off-by: Nir Soffer <nsoffer@redhat.com>

* Improve API to get flag completion function (spf13#2063)

The new API is simpler and matches the `c.RegisterFlagCompletionFunc()`
API.  By removing the global function `GetFlagCompletion()` we are more
future proof if we ever move from a global map of flag completion
functions to something associated with the command.

The commit also makes this API work with persistent flags by using
`c.Flag(flagName)` instead of `c.Flags().Lookup(flagName)`.

The commit also adds unit tests.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

* feat: expose GetCompletions (was getCompletions)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: David Wertenteil <dwertent@armosec.io>
Signed-off-by: John McBride <jpmmcbride@gmail.com>
Signed-off-by: Oldřich Jedlička <oldium.pro@gmail.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Luiz Carvalho <lucarval@redhat.com>
Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
Signed-off-by: Taavi Väänänen <hi@taavi.wtf>
Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Signed-off-by: Volodymyr Khoroz <volodymyr.khoroz@foundries.io>
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
Co-authored-by: Brian Pursley <bpursley@cinlogic.com>
Co-authored-by: Enrico Candino <enrico.candino@gmail.com>
Co-authored-by: Norman Dankert <norman.dankert@outlook.com>
Co-authored-by: Unai Martinez-Corral <38422348+umarcor@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: David Wertenteil <dwertent@armosec.io>
Co-authored-by: Yash Ladha <18033231+yashLadha@users.noreply.github.com>
Co-authored-by: Seonghyeon Cho <seonghyeoncho96@gmail.com>
Co-authored-by: Dominik Roos <domi.roos@gmail.com>
Co-authored-by: Shihta Kuan <Shihta@users.noreply.github.com>
Co-authored-by: janhn <jan@hnatek.eu>
Co-authored-by: Ggg6542 <465806+gusega@users.noreply.github.com>
Co-authored-by: John McBride <jpmmcbride@gmail.com>
Co-authored-by: Gyanendra Mishra <anomaly.the@gmail.com>
Co-authored-by: Oldřich Jedlička <oldium@users.noreply.github.com>
Co-authored-by: Oldřich Jedlička <oldrich.jedlicka@rohlik.cz>
Co-authored-by: Florent Poinsard <35779988+frouioui@users.noreply.github.com>
Co-authored-by: Luiz Carvalho <luizcarvalho85@gmail.com>
Co-authored-by: Marc Khouzam <marc.khouzam@gmail.com>
Co-authored-by: Deleplace <deleplace2015@gmail.com>
Co-authored-by: Tom Payne <tom.payne@flarm.com>
Co-authored-by: Taavi Väänänen <hi@taavi.wtf>
Co-authored-by: Branch Vincent <branchevincent@gmail.com>
Co-authored-by: Paul Holzinger <45212748+Luap99@users.noreply.github.com>
Co-authored-by: Martijn Evers <94963229+marevers@users.noreply.github.com>
Co-authored-by: gocurr <xigua67damn@gmail.com>
Co-authored-by: Jun Nishimura <n.junjun0303@gmail.com>
Co-authored-by: Nuno Adrego <55922671+nunoadrego@users.noreply.github.com>
Co-authored-by: Souma <101255979+5ouma@users.noreply.github.com>
Co-authored-by: Alexandru-Claudius Virtopeanu <alexandru-claudius.virtopeanu@ionos.com>
Co-authored-by: Haoming Meng <41393704+Techming@users.noreply.github.com>
Co-authored-by: vkhoroz <vkhoroz@users.noreply.github.com>
Co-authored-by: Ville Skyttä <ville.skytta@upcloud.com>
Co-authored-by: Nir Soffer <nirsof@gmail.com>
Co-authored-by: Geoffrey Ragot <geoffrey.ragot@gmail.com>
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 a pull request may close this issue.

2 participants