microsoft / Documentarian

An open source toolkit for documentarians and community contributors to reduce friction and provide a delightful experience for contributing to and maintaining documentation.
Creative Commons Attribution 4.0 International
37 stars 9 forks source link

Ensure all public functions define and adhere to the `OutputType` attribute #118

Open michaeltlombardi opened 1 year ago

michaeltlombardi commented 1 year ago

Prerequisites

Module

Documentarian, Documentarian.DevX, Documentarian.MarkdownLint, Documentarian.MicrosoftDocs, Documentarian.ModuleAuthor, Documentarian.Vale

Summary

As a user, I want the best possible IntelliSense and validation when using the Documentarian modules.

Every public function should declare the OutputType attribute, even if they don't return any objects by default.

Details

When commands define their output type correctly, IntelliSense in the terminal and in editors can help users to write PowerShell code that processes the output of those commands. Without the output types defined, the users need to cast the output explicitly themselves to get IntelliSense to function.

Further, documenting the output types for a command adds sections for those outputs to the documentation, making it easier for users to discover and understand how, why, and when a command returns different objects.

santisq commented 1 year ago

I might be able to help with this, I'm not so sure where would you be performing this validation, I assume it should throw during the build process, but this snippet might help you finding functions that don't have an OutputType decoration (using a scriptblock for testing, it should be changed to $ast = [Parser]::ParseFile($ps1path, [ref] $null, [ref] $null)... if implemented).

using namespace System.Management.Automation
using namespace System.Management.Automation.Language

$ast = {
    function Test-Function {
        [CmdletBinding()]
        param()
    }

    function Test-FunctionWithOutputType {
        [OutputType([object])]
        param()
    }
}.Ast.FindAll({ $args[0] -is [FunctionDefinitionAst] }, $false) # <= `$false` here so it doesn't look for inner functions..

foreach ($funcAst in $ast) {
    $hasOutputType = $funcAst.Find({
        $args[0] -is [AttributeAst] -and
        $args[0].TypeName.GetReflectionType() -eq [OutputType] }, $true)

    if (-not $hasOutputType) {
        $funcAst.Name
    }
}

Edit: Actually this is much simpler, thanks to @seeminglyscience for the tip 😅


....FindAll({ $args[0] -is [FunctionDefinitionAst] }, $false) |
    Where-Object { $_.Body.ParamBlock.Attributes.TypeName.GetReflectionType() -notcontains [OutputType] } |
    ForEach-Object Name
michaeltlombardi commented 1 year ago

I might be able to help with this, I'm not so sure where would you be performing this validation, I assume it should throw during the build process

Eventually, I think we'll want it to be in the acceptance tests or something. For now, we're just doing a manual pass to get everything we have updated. One of the things we haven't added in DevX yet, but I eventually want to make easier, are modularized test sets, particularly for module development practices. I think a test like this is a great fit for that eventuality.

this snippet might help you finding functions that don't have an OutputType decoration

Actually, it occurs to me that this is a great fit for a function, like Test-OutputType - first implementation could just be "does the thing I'm inspecting define an output type at all?" - we could iterate later to see if specific output types are defined, or if the tested definition defines the correct output types, etc.

santisq commented 1 year ago

I might be able to help with this, I'm not so sure where would you be performing this validation, I assume it should throw during the build process

Eventually, I think we'll want it to be in the acceptance tests or something. For now, we're just doing a manual pass to get everything we have updated. One of the things we haven't added in DevX yet, but I eventually want to make easier, are modularized test sets, particularly for module development practices. I think a test like this is a great fit for that eventuality.

this snippet might help you finding functions that don't have an OutputType decoration

Actually, it occurs to me that this is a great fit for a function, like Test-OutputType - first implementation could just be "does the thing I'm inspecting define an output type at all?" - we could iterate later to see if specific output types are defined, or if the tested definition defines the correct output types, etc.

@michaeltlombardi I'll be glad to help (my retribution for the awesome docs 😉). What should Test-OutputType take as input? a ScriptBlock ? a path to a .ps1 file ? a CommandInfo ? whenever you have this defined let me know and I'll try to help.

michaeltlombardi commented 1 year ago

@santisq - I think you could probably borrow the parameter sets from Get-Ast for this purpose, as that function operates on files, scriptblocks, and raw text:

https://github.com/microsoft/Documentarian/blob/a3f5bf6f3c32a4a747943dba5f5255bcd1cfb5f3/Projects/Modules/Documentarian.DevX/Source/Private/Functions/Get-Ast.ps1#L10-L21

And add the option for supporting CommandInfo or Ast - we have a potentially useful set of private transformers and validators for this as well that you could use for inspiration: