PoshCode / ModuleBuilder

A PowerShell Module to help scripters write, version, sign, package, and publish.
MIT License
445 stars 54 forks source link

Add support for exporting aliases using *-Alias in public scripts #104

Closed johlju closed 3 years ago

johlju commented 3 years ago

This PR adds to the behavior of exporting aliases. It will export aliases declared with [Alias()] but it is now also possible to export aliases using New-Alias and Set-Alias that are defined in separate script file (at script level) in a public script (in Public folder). It will also handle if Remove-Alias is used and will assume that alias should not be exported.

Prior to this change GetCommandAlias was passed an AST for the entire built module and parsed the AST for [Alias()]. In this change the AST is also parsed for the commands New-Alias, Set-Alias, and Remove-Alias. If a New-Alias, Set-Alias is part of a file that is placed in the folder Public and have the same basename as the alias name it will be exported (assuming the file name is filtered as part of the PublicFilter).


This change is Reviewable

Jaykul commented 3 years ago

I still think it needs to check for the scope parameter on New-Alias and Set-Alias.

One of the few reasons I would use the alias commands instead of the attribute would be to explicitly put aliases directly into Global scope, and those aliases don't need to be "exported" in the manifest:

New-Item -Type Directory TestModules\Foo
@'
function test-foo {
  [CmdletBinding()]
  [Alias('tf')]
  param($foo)
    Write-Host "Is it foo?"
    $foo -eq "foo"
}

New-Alias New-Foo Get-Command -Scope Global
'@ > TestModules\Foo\Foo.psm1

New-ModuleManifest TestModules\Foo\Foo.psd1 -RootModule foo.psm1 -FunctionsToExport Test-Foo

$Env:PSModulePath += ';' + (convert-path .\TestModules\)

Import-Module Foo -Passthru | Format-List # See only the `tf` alias
Get-Alias New-Foo # But this alias _does_ exist!

image

johlju commented 3 years ago

I still think it needs to check for the scope parameter on New-Alias and Set-Alias.

Yes, I missed what you meant with the scope. Got it now, can fix that. When we have solve where to put the New-Alias and can differentiate between a public New-Alias and a private New-Alias (that does not use scope Gobal). See more in my comment on your review comment.

Thank you for taking the time discussing this! πŸ™‚

gaelcolas commented 3 years ago

To add a bit more color to what @johlju suggests, I just want to make it clear why. InvokeBuild uses this way to "expose" extensions and discover them through the manifest (in this case build tasks). For the lack of better community-agreed practices, it seems to do the job. I'm also not a fan of having it in the public/private folder, and would rather it to be in prefix/suffix, but in the end, all the one AST found not in Private folder should be exportable & populate the module manifest. Happy for needing a switch if you think it's really necessary.

Jaykul commented 3 years ago

@johlju Can you add some test coverage for this feature? Ideally, a unit test of the functionality and the -scope exclusion in GetCommandAlias.Tests and an integration test that shows how the public filter affects it.

We might also want to rename "IgnoreAliasAttribute" to "IgnoreAlias" (and put an [Alias("IgnoreAliasAttribute")] on it for backward compatibility), what do you think?

johlju commented 3 years ago

Absolutely, I fix tests this weekend. I agree we should change IgnoreAliasAttribute bas you suggest. I fix that too. I ping when it is ready again. πŸ™‚

johlju commented 3 years ago

@Jaykul Now I have fixed the integration tests and made the appropriate changes to Build-Modul. I hope you are satisfied with the changes that are proposed here. πŸ™‚ Thank you.

Ps. Sorry for the all the commits, but I couldn't add the pipeline to my own Azure DevOps project since the pipeline used configuration from a separate project that my pipeline didn't have permission to use. Ds.

Jaykul commented 3 years ago

I'll definitely have to see why you can't use the pipeline tasks, I thought they were shared enough, but maybe I have to publish to the marketplace (I didn't really think they were ready for that).

I'll look at this this weekend, sorry it took so long.