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

DontShow should not hide common parameters #18472

Open
MartinGC94 opened this issue Nov 5, 2022 · 10 comments · May be fixed by #19393
Open

DontShow should not hide common parameters #18472

MartinGC94 opened this issue Nov 5, 2022 · 10 comments · May be fixed by #19393
Labels
In-PR Indicates that a PR is out for the issue Issue-Enhancement the issue is more of a feature request than a bug Resolution-No Activity Issue has had no activity for 6 months or more WG-Cmdlets general cmdlet issues WG-Interactive-IntelliSense tab completion

Comments

@MartinGC94
Copy link
Contributor

Summary of the new feature / enhancement

The DontShow attribute parameter that the Parameter attribute has allows a function/script author to hide parameters like this:

function Verb-Noun
{
    Param
    (
        [Parameter()]
        [string]
        $Param1,

        [Parameter(DontShow)]
        [string]
        $Param2
    )
}
#Tab completion does not show Param2
Verb-Noun -<Tab>

This has the side effect of also hiding the common parameters.
It seems to have been an intentional design choice, see this: https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs#L768 but I think that should be changed because it makes it impractical to use DontShow to hide old deprecated parameters like ConvertTo-Csv does with the NoTypeInformation parameter because then the user won't see important common parameters like ErrorAction.

Proposed technical implementation details (optional)

Instead of automatically hiding common parameters when DontShow has been specified, add a HideCommonParameters parameter to the CmdletBinding attribute.

@MartinGC94 MartinGC94 added Issue-Enhancement the issue is more of a feature request than a bug Needs-Triage The issue is new and needs to be triaged by a work group. labels Nov 5, 2022
@kilasuit
Copy link
Collaborator

Alternatively a change on L780 from false to true would be the simplest way to solve this This has the side effect of also hiding the common parameters.

if (compiledAttributes != null && compiledAttributes.Count > 0)
{
foreach (var attr in compiledAttributes)
{
var pattr = attr as ParameterAttribute;
if (pattr != null && pattr.DontShow)
{
showToUser = false;
addCommonParameters = false;
break;

However expanding on your proposed implementation this I think that if a HideCommonParameters option to CmdletBinding was added then there would need to be a check for it.
My question would be Is there really a need for us to hide the common parameters? & if not then do we really need a HideCommonParameters option for the CmdletBinding attribute?

@kilasuit kilasuit added the WG-Interactive-IntelliSense tab completion label Nov 10, 2022
@MartinGC94
Copy link
Contributor Author

I can see why someone might want to hide the 11 common parameters you get with cmdletbinding in simple functions/scripts that are meant to be used by less technical users. So I think it's best to have a replacement instead of just completely removing the option.

@kilasuit
Copy link
Collaborator

kilasuit commented Nov 10, 2022

I can see why someone might want to hide the 11 common parameters you get with cmdletbinding in simple functions/scripts that are meant to be used by less technical users.

For Simple scripts/functions you can do this by omitting the [parameter()] attribute off of all parameters entirely

function Verb-Nounsimple { 
    Param
    (
        [string]
        $Param1,

        [string]
        $Param2
    )
}

or also commonly written like so

function Verb-Nounsimple { param([string]$Param1,[string]$Param2) }

wont give you the common parameters in tab completion

Same applies if you were to have a script with simple param block as per the above verb-nounsimple example - which is why I asked the question I did, because we already have the ability inbox, unless there really is a use case for the following

  • Hide a parameter from intellisense & also hide common parameters too

in which case the current design makes sense
However I don't think it does and think that the correct fix is the simplest one in the change I highlighted in L780
That said that is unless you want an advanced function / script where you can Hide a parameter from intellisense but choose to either hide or show common parameters too - is this really the ask & if so then how much of a need is there for this, I'm guessing that it's not a critical need, more a nice to have and not likely to be a high priority for the team to take on

@MartinGC94
Copy link
Contributor Author

Sure you can, but then you miss out on pipeline support, parametersets, and mandatory parameters.

@kilasuit
Copy link
Collaborator

Sure you can, but then you miss out on pipeline support, parametersets, and mandatory parameters.

Then you want an advanced function/script and not a simple function / script which again comes back to a more important question on function/script design. which is a much larger question than many realise until later on in the development of it & often changes drastically over time.

@MartinGC94
Copy link
Contributor Author

Sure you can, but then you miss out on pipeline support, parametersets, and mandatory parameters.

Then you want an advanced function/script and not a simple function / script which again comes back to a more important question on function/script design. which is a much larger question than many realise until later on in the development of it & often changes drastically over time.

So what's your point here? That nobody would want to hide the common parameters for a random script with a mandatory parameter?
I'm sure I'm not the only one that has done something like this in a script where I needed a mandatory parameter but didn't want to confuse the user with the common parameters:

Param
(
    [Parameter(Mandatory)]
    [string]
    $Param1,

    # Dummy parameter to hide common parameters
    [Parameter(DontShow)]
    $Dummy
)

Just to be clear here, I'm obviously not arguing to keep the current behavior, I'm just saying that if we fix the current behavior we need to provide an alternative for the people that actually wants to hide common parameters.

@dwtaber
Copy link
Contributor

dwtaber commented Nov 11, 2022

Sure you can, but then you miss out on pipeline support, parametersets, and mandatory parameters.

Then you want an advanced function/script and not a simple function / script which again comes back to a more important question on function/script design. which is a much larger question than many realise until later on in the development of it & often changes drastically over time.

Sometimes one writes advanced functions for simple users.

I agree with @MartinGC94 that there's value in separating DontShow from hiding common parameters and keeping both capabilities. Personally, for the use-case of making things less overwhelming for less skilled users, I think I'd rather see that handled by something like a $CommonParameterVisibility preference variable, rather than having to deal with it on a per-function basis.

@theJasonHelmick
Copy link
Collaborator

The Working Group has reviewed this -- we will investigate the original decision. The general feeling is that a parameter attribute should not affect other parameters or the common parameters. However, we would like to investigate the original reasoning behind disabling common parameters before making any changes - as it is very clear it was the desired behavior at the time.

@theJasonHelmick theJasonHelmick removed the Needs-Triage The issue is new and needs to be triaged by a work group. label Dec 7, 2022
@MartinGC94 MartinGC94 linked a pull request Mar 22, 2023 that will close this issue
22 tasks
@ghost ghost added the In-PR Indicates that a PR is out for the issue label Mar 22, 2023
@daxian-dbw daxian-dbw added the WG-Cmdlets general cmdlet issues label May 1, 2023
@MartinGC94
Copy link
Contributor Author

@theJasonHelmick Any update from the working group?

Copy link
Contributor

This issue has not had any activity in 6 months, if there is no further activity in 7 days, the issue will be closed automatically.

Activity in this case refers only to comments on the issue. If the issue is closed and you are the author, you can re-open the issue using the button below. Please add more information to be considered during retriage. If you are not the author but the issue is impacting you after it has been closed, please submit a new issue with updated details and a link to this issue and the original.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Resolution-No Activity Issue has had no activity for 6 months or more label May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In-PR Indicates that a PR is out for the issue Issue-Enhancement the issue is more of a feature request than a bug Resolution-No Activity Issue has had no activity for 6 months or more WG-Cmdlets general cmdlet issues WG-Interactive-IntelliSense tab completion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@daxian-dbw @kilasuit @theJasonHelmick @dwtaber @MartinGC94 and others