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 [email protected]
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.

[rodmhgl]

@Jaykul Would you mind expanding a bit on 'throwing inside your function is awful'. I've often wondered where error checking should be attempted (inside the function or when I call the function?) and would love to see this expanded on in the guide. A bit off-track, but while we're talking about it...

[dlwyatt]

I frequently use non-advanced functions internally (unless I want to support multiple parameter sets), though everything exported is Advanced.

I'm not really sure why I do it; it just seems unnecessary to make a function "advanced" when it will already inherit the $VerbosePreference / etc values from the calling function anyway, and I have no intention of calling my own internal function with an explicit -Verbose value, as an example. This mainly goes for functions that I write purely to increase readability; giving a meaningful name to an expression. For example:

function Get-RelativePath([string] $Path, [string] $RelativeTo )
{
    $RelativeTo = $RelativeTo -replace '\\+$'
    return $Path -replace "^$([regex]::Escape($RelativeTo))\\?"
}

There's no reason to make this function advanced. It can literally never throw an exception, even if the input arguments are empty. It can't produce any Verbose or Warning output (and doesn't need to). It just exists because the calling code makes a lot more sense when you see something like this:

$relativePath = Get-RelativePath -Path $sourceFile.FullName -RelativeTo $sourceRootDirectory.FullName
$targetPath = Join-Path $targetRootDirectory.FullName $relativePath

As opposed to seeing a couple of regex / -replace operators and having to try to understand at a glance what it's doing.

[dlwyatt]

I guess another way of putting it is this: If I, the module author, control all of the calls to a function, then I can write that function however the hell I want to (and this goes for function name, parameter names, advanced / non-advanced, etc.) :) If I'm providing a function as an API to someone else, then I would hold that function to a higher standard.

[nightroman]

@Jaykul

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.

That is the point of my argument. These rules declare things that serve me well
as not recommended. But do the rules really provide enough reasons?

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.

The quality of this approach is another issue, useful to discuss, by the way.
But I just said that a statement was not exactly true and provided an example
how Verbose and etc. can be reported as errors if this is really needed. When
something is really needed then even not perfect way may be fine. Besides, I
find the problem with Verbose and etc. rather academic. Personally, I will
never call my internal simple functions with such parameters by mistake.

[nightroman]

@dlwyatt explained this even better and with a real example.

If I, the module author, control all of the calls to a function, then I can write that function however the hell I want to (and this goes for function name, parameter names, advanced / non-advanced, etc.)

Exactly. I still think though that some rules should be recommended even for internal function names. Too simple function names may clash with existing aliases, even internal module functions may conflict with global aliases. As for a script function, it may conflict with any existing alias. I have seen this problem in practice quite often, it is real. Problematic aliases may not exist in the development environment but when a tool goes to users then such aliases may exist.

[leesoh]

A bit late to the party here, but thought I'd add my $0.02.

If I, the module author, control all of the calls to a function, then I can write that function however the >hell I want to (and this goes for function name, parameter names, advanced / non-advanced, etc.)

I'm far less advanced of a user than it sounds like you guys are, but one of the biggest strengths of PowerShell is, in my opinion, the huge amount of community resources available. Why not write your code in such a way that, if you ever decided to to share it, or someone else had to take over maintenance, it would be simple, easy to understand, and portable. It seems to me that CmdletBinding meets these criteria with fairly minimal overhead.

[Jaykul]

I agree with @Li-So here, but just to be clear:

Nothing in this guideline is meant to prevent people from leaving things out on purpose. You can write code however you like. Best practice documents and style guides have never stopped anyone from taking shortcuts or being productive.

The goal of this document isn't to control you, or even to shame you.

The goal of this document is to help people who want help to write better, more readable, more maintainable code.

If anyone honestly thinks that creating a second, lesser standard for internal functions with different recommendations for naming conventions, parameter handling, etc will result in better code, with less errors, that is more readable and more maintainable than sticking the same standard to all functions ... feel free to make your case. I am pretty confident that a single set of best practices for functions is better.

I will reiterate: "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," and given that, I still think that the guideline should be:

Always use CmdletBinding. Functions without CmdletBinding and common parameters do not behave the way that users (or the maintainer who comes after you) expect them to. They don't throw errors when you call them with common parameters (like -Verbose or -ErrorVariable), but they don't work as expected either. In addition, without the CmdletBinding attribute, it's possible to have an "advanced function" with common parameters that doesn't look advanced, since PowerShell will automatically promote your function if you use certain attributes on the parameters.

I'm always happy to add an additional paragraph like this, if it will help make anyone comfortable @nightroman @KirkMunro @dlwyatt -- the exceptions are always implied (that is, I've written something very much like this in several places already, and I'm trying not to repeat it on every page), but when there's a specific counter-case example, it's worth adding.

As with all the guidelines in this document, there may be exceptions, and even cases when leaving off the common parameters is necessary in order to achieve a desired effect -- whether that's to style a DSL a certain way, or to wrap APIs or legacy executable command-line apps to produce a compatible syntax to their bash equivalents. Just keep in mind that these rules are intended to help you write code that is easier to read and maintain, and harder to screw up -- even for the guy that comes after you. Use CmdletBinding as your default, and only leave it off when you're trying to achieve something specific.

[KirkMunro]

I agree with all of that.

I also think, as far as having a best practice/style guide goes, that a
best practice for the exceptions to the rule is to include comments that
explain why it is necessary to be exceptional in those cases. That may be
worth adding as a general statement for the entire best practice/style
guide, wherever it would be most appropriate.

*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, Jul 26, 2015 at 2:17 PM, Joel Bennett [email protected]
wrote:

I agree with @Li-So https://github.com/li-so here, but just to be
clear:

Nothing in this guideline is meant to prevent people from writing sloppy
code on purpose. You can write code however you like. Best practice
documents and style guides have never stopped anyone from taking shortcuts
or "being productive."

The goal of this document isn't to control you, or even to shame you.
The goal of this document is to help people who want help to write
better, more readable, more maintainable code.

If anyone honestly thinks that creating a second, lesser standard for
internal functions with different recommendations for naming conventions,
parameter handling, etc will result in better code, with less errors, that
is more readable and more maintainable than sticking the same standard to
all functions ... feel free to make your case. I am pretty confident that a
single set of best practices for functions is better.

I will reiterate: "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," and given that, I still think that the
guideline should be:

Always use CmdletBinding. Functions without CmdletBinding and common
parameters do not behave the way that users (or the maintainer who comes
after you) expect them to. They don't throw errors when you call them with
common parameters (like -Verbose or -ErrorVariable), but they don't work as
expected either. In addition, without the CmdletBinding attribute, it's
possible to have an "advanced function" with common parameters that doesn't
look advanced, since PowerShell will automatically promote your function if
you use certain attributes on the parameters.

I'm always happy to add an additional paragraph like this, if it will help
make anyone comfortable @nightroman https://github.com/nightroman
@KirkMunro https://github.com/KirkMunro @dlwyatt
https://github.com/dlwyatt -- the exceptions are always implied (that
is, I've written something very much like this in several places already,
and I'm trying not to repeat it on every page), but when there's a specific
counter-case example, it's worth adding.

As with all the guidelines in this document, there may be exceptions, and
even cases when leaving off the common parameters is necessary in order
to achieve a desired effect -- whether that's to style a DSL a certain way,
or to wrap APIs or legacy executable command-line apps to produce a
compatible syntax to their bash equivalents. Just keep in mind that these
rules are intended to help you write code that is easier to read and
maintain, and harder to screw up -- even for the guy that comes after you.
Use CmdletBinding as your default, and only leave it off when you're trying
to achieve something specific.

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

[rkeithhill]

You know, the .NET Framework Design Guidelines book has many rule "annotations" from folks like Jeff Richter, Brad Abrams, Pattrick Dussud, etc. While the rules are usually pretty straight forward do's and don'ts - these annotation sidebars often provide very useful nuanced wisdom on particular rules. If you're in a bookstore (or have the book on your bookshelf), check it out. It might be a useful concept to use on these guidelines. Perhaps you could persuade some PowerShell team members and MVPs to provide annotations.

BTW one annotation I would add (or perhaps this is a rule):

When declaring a parameter that will use pipeline binding, be sure to make the parameter type accept an array. If you don't then while pipelining will work:

Get-ChildItem -File | Set-Writable

This will not work because the parameter, let's call it LiteralPath, does not accept an array of string:

Set-Writable foo.txt, bar.txt, baz.txt

[Jaykul]

I don't know @rkeithhill -- that's definitely conditional on which parameter it is -- the built-in cmdlets never do that for InputObject, for instance, and you should only do it for the main parameter. Right?

[rkeithhill]

@Jaykul Yeah it needs a bit more nuance such as this should be done for the "main" parameter (however we define main parameter). And for the parameter type PSObject - like .NET object - it allows for arrays to be assigned to it as well as scalars e.g.:

foreach-object -inp 1,2,3 {$_}

So in this case it isn't necessary to declare the parameter type as [PSObject[]] since [PSObject] will suffice. But I think PSObject is the exception.

The thing is, if you don't do this then your users will be sorely disappointed when something as simple as Set-Writable foo.txt, bar.txt doesn't work simply because the author, who went to all the trouble to make the command accept pipeline input, declared the Path parameter as [string] instead of [string[]]. I think that would be an easy mistake for folks to make.

[Jaykul]

Yeah, I'm still upset that commands like Format-* and Out-* and ConvertTo* and Sort/Tee/Select don't work properly with arrays 😠 but since Microsoft has stated that enumerating an array passed to InputObject would be wrong, the decision is complicated 😢 and there's obviously a good reason (occasionally) to say: "my command is like Measure-Object, so if you want to support multiple things, you have to pipe them" (and therefore, I'm not sure what to say about it).