Should the Style Guide recommend against non-advanced functions?

[Jaykul]

https://github.com/PoshCode/PowerShellPracticeAndStyle/blob/master/Style%20Guide/English.md#functions

I think the Style Guide should just say:

Prefer CmdletBinding because functions without CmdletBinding don't get common parameters and therefore do not behave the way users expect them to. For instance, when you call them with common parameters (like -Verbose or -WhatIf or -ErrorVariable), those parameters don't work as expected either.

I could add a few other reasons, mostly around the inconsistency and the fact that you frequently have to upgrade functions to CmdletBinding which changes their syntax, but my point is that I think the style guide and best practices should strongly recommend writing advanced functions, rather then providing style suggestions for them.

Anyone want to defend the use of non-advanced functions?

[nightroman]

I agree if this is about public functions. Internal non-advanced functions are up to a developer. Non-advanced functions are simpler sometimes.

[KirkMunro]

I have found scenarios where I use non-advanced functions simply because
they allow me to do things that advanced functions cannot. For example,
creating a function that takes a variable number of arguments where you
don't even want a parameter name to result in something being excluded from
the arguments passed in. You can do that with $args in regular functions,
and with advanced functions having to name the parameter that captures all
arguments puts the name of that parameter in the way. In general, all
publicly exported functions should be advanced, but internally you may have
functions with other requirements beyond cmdlet binding and common
parameters. All to say, I agree with Roman.

*Kirk Munro *
Poshoholic, Microsoft MVP
Blog http://poshoholic.com/ | Twitter http://twitter.com/poshoholic |
LinkedIn http://ca.linkedin.com/in/kirkmunro | GitHub
http://github.com/KirkMunro | Facebook
http://www.facebook.com/#%21/kirk.munro

Need a PowerShell SME for a project at work? Contact me
http://poshoholic.com/contact-me/!

On Sun, May 31, 2015 at 2:16 AM, Roman Kuzmin notifications@github.com
wrote:

I agree if this is about public functions. Internal non-advanced functions
are up to a developer. Non-advanced functions are simpler sometimes.

—
Reply to this email directly or view it on GitHub
#32 (comment)
.

[Jaykul]

Needless to say, I think you're wrong -- I've been bitten by the "why doesn't -<commonparameter> work" problem too many times (especially when working with other people's code).

I agree that they're simpler to write, but there's not enough value in not having to write this:

param([Parameter(ValueFromRemainingArguments)]$args)

For one-off stuff while you're tinkering or developing or writing at the console, of course it doesn't matter, but once you are saving them (even if they're "just" internal functions) there's a possibility of collaboration or of someone else taking over after you. At that point, I don't think the "simplicity" is worth the complexity of dealing with two different syntaxes and command behaviors.

@KirkMunro last week you were arguing we should use Verb-Noun for all functions because we might decide to export them, and now you want to write them with $args and $_ and $input -- I find that completely contradictory. 😕

However, if we're not going to explicitly forbid them, then do we need specific style guidance for them?

1. Do you name them specially so you can remember they don't support common parameters?
2. Do you insist on a param() block being inside the function even though there's no reason for it in this case?
3. Do you forbid pipeline process blocks since you can't specify pipeline parameter names?

[nightroman]

@Jaykul here are some examples. Why do they need special names? Why should they have param? Why should process block be forbidden?

filter Get-Even {
    if ($_ % 2 -eq 0) {$_}
}

1..10 | Get-Even

function Get-Odd {
    process {
        if ($_ % 2) {$_}
    }
}

1..10 | Get-Odd

[KirkMunro]

@Jaykul: I didn't say it was the norm for me to create functions that are not advanced functions. But valid use cases for these functions still exist -- scenarios where advanced functions simply don't work.

Since you called me on it, I went digging and found the use case I couldn't recall on the weekend. I have a function that wraps plink.exe. It is named Invoke-Plink (best practice). It has an associated alias, "plink" (so that it works as is with plink commands copied from elsewhere). And if I make it an advanced function, it won't work (I forget the exact details but I believe it had to do with passing in pipeline input to the executable that is being wrapped). So I'm not contradicting anything I said about using Verb-Noun because we might want to export them.

I'm simply saying that the world of automation is not as cut and dry as a style guide or best practice guide creator/author might like it to be. Regular functions still have their place, and in some cases they are absolutely necessary, so they absolutely should not be forbidden.

[KirkMunro]

One more thing: for internal functions, supporting/not supporting common parameters is not much of a problem, is it, since the public functions that invoke them will result in $*Preference variables being set which in turn would allow for the appropriate behaviour to take place inside the internal function should someone want it to.

[nightroman]

And let's take it to the extreme. We can use script blocks instead of functions, sometimes they are more convenient. Should they also be only advanced?

[mattmcnabb]

I've definitely used scriptblocks in cases where a function didn't make sense, for no other reason than to help organize the code.

Remember that this is a style guide, not a style canon. The goal is to recommend a set of practices. If a particular practice is not preferred, the best bet is simply to explain why. Qualify the statements and that will leave readers with the knowledge to decide when these guidelines should be adhered to and when they can be bent.

[rkeithhill]

RE not a style canon - yeah I agree with that sentiment. Let's not go crazy with the Cheez Whiz here. Let's only offer style guidance (or recommend against a style) when there is a real benefit.

[Jaykul]

Yeah, hang on a sec. This isn't a series of laws or mandatory practices.

But the fact that you can come up with an exception to a rule doesn't mean
that rule is invalid. Programming is like English, EVERY good rule has an
exception. In my suggested format for these rules (which got lost in the
shuffle when I brought in the book) I specifically suggested that every
rule should have an "except when" clause.

So my point is not that you should never have a script block or filter, but
that we should recommend advanced functions as the best practice and that
"simple" functions should be the exception because (all the reasons above),
except when ... (however you want to phrase "it's internal and simple, and
you're sure you won't want to export it", or whatever other exception you can come up with).

[nightroman]

Always use CmdletBinding

Sounds like a law :)

[nightroman]

They don't throw errors when you call them with common parameters (like -Verbose or -ErrorVariable), but they don't work as expected either.

This is not exactly true if a function is properly designed, for example:

    function Test-Param($Param1) {
        if ($args) {Write-Error -ErrorAction Stop "Invalid arguments: $args"}
        "Param1 is $Param1"
    }

    # correct
    Test-Param -Par Value1

    # invalid
    Test-Param -Pram1 Value1

Output:

Param1 is Value1

Test-Param : Invalid arguments: -Pram1 Value1
At ...\Test.ps1:11 char:1
+ Test-Param -Pram1 Value1

Or if we try -Verbose or -ErrorVariable

    Test-Param -Param1 Value1 -Verbose

we'll get

    Test-Param : Invalid arguments: -Verbose
    At ...\Test.ps1:8 char:1
    + Test-Param -Param1 Value1 -Verbose

[Jaykul]

@nightroman That there is ... well, combine that with your where-even filter function, and I'd say you're in violation of at least three or four best practices. Testing args and throwing inside your function is awful: it results in every function having a different non-localized error message for the same problem.