Linting

[jfhbrook]

Hello!

I was setting up a PowerShell environment in Linux and I was using your module. I noticed that Get-NodeVersions ended in a plural - afaik PowerShell's style guide says that singular nouns are preferred over plural ones even when the command can return multiple things. So, I added an alias so that Get-NodeVersion works as well.

I also went ahead and added directions for linting, addressed the most obvious linting issues - this was changing a variable name and adding ShouldProcess support to a few of the cmdlets - and updated the tests to run against Pester 5.

I should note that the tests don't work for me - I get a bunch of errors:

PS /home/josh/software/jfhbrook/ps-nvm> Invoke-Pester

Starting discovery in 1 files.
Discovery finished in 65ms.
[-] Set-NodeVersion.auto-discovery.Will set from the .nvmrc file 26ms (25ms|1ms)
 RuntimeException: The variable '$nodeVersion' cannot be retrieved because it has not been set.
 at <ScriptBlock>, /home/josh/software/jfhbrook/ps-nvm/nvm.tests.ps1:266
[-] Set-NodeVersion.auto-discovery.Will set from the default file 32ms (31ms|1ms)
 RuntimeException: Version not given, no .nvmrc found in folder, and package.json missing or does not contain node engines field
 at Get-TargetNodeVersion, /home/josh/software/jfhbrook/ps-nvm/nvm.psm1:173
 at Set-NodeVersion, /home/josh/software/jfhbrook/ps-nvm/nvm.psm1:82
 at <ScriptBlock>, /home/josh/software/jfhbrook/ps-nvm/nvm.tests.ps1:300
[-] Set-NodeVersion.Set from version string.Will set from the supplied version 9ms (8ms|1ms)
 RuntimeException: The variable '$nodeVersion' cannot be retrieved because it has not been set.
 at <ScriptBlock>, /home/josh/software/jfhbrook/ps-nvm/nvm.tests.ps1:332
[-] Set-NodeVersion.Set from version string.Will set from a version range 45ms (44ms|1ms)
 RuntimeException: The variable '$nodeVersion' cannot be retrieved because it has not been set.
 at <ScriptBlock>, /home/josh/software/jfhbrook/ps-nvm/nvm.tests.ps1:340
[-] Set-NodeVersion.Set from version string.Will set from a version range with caret 43ms (42ms|1ms)
 RuntimeException: The variable '$nodeVersion' cannot be retrieved because it has not been set.
 at <ScriptBlock>, /home/josh/software/jfhbrook/ps-nvm/nvm.tests.ps1:347
[-] Set-NodeVersion.pipeline.Will set from the supplied version via Install-NodeVersion pipeline output 19ms (18ms|1ms)
 RuntimeException: The variable '$nodeVersion' cannot be retrieved because it has not been set.
 at <ScriptBlock>, /home/josh/software/jfhbrook/ps-nvm/nvm.tests.ps1:405
[-] Set-NodeVersion.pipeline.Will set from the supplied version via Get-NodeVersion pipeline output 3ms (2ms|1ms)
 RuntimeException: The variable '$nodeVersion' cannot be retrieved because it has not been set.
 at <ScriptBlock>, /home/josh/software/jfhbrook/ps-nvm/nvm.tests.ps1:409
Tests completed in 999ms
Tests Passed: 16, Failed: 7, Skipped: 13 NotRun: 0

I get these same errors whether or not I run against master, patch to make tests run against v5 notwithstanding, so I'm fairly certain these changes don't introduce any bugs.

Cheers!

[aaronpowell]

It looks like whatever has been done to upgrade the Pester version has removed the ability to generate CodeCoverage results, so the tests don't even run.

With regards to the naming convention of Get-NodeVersions, this is one time where I strongly disagree with a convention.

When I read Get-NodeVersion I expect that it returns the version of the currently selected version of node, essentially returning node --version, whereas what Get-NodeVersions does is returns all the available node versions to select from. And it would be even more confusing with the -Remote flag, as a singular naming but with a remote flag and no filter returns what?

In fact, #81 made the request for a way to get the current node version using a Get-NodeVersion commandlet, but I didn't see the value in it.

I can't think of a way to make the singular naming convention match up with the intent of the commandlet, or how to write a commandlet name that is singular but has a clearly defined intent that it returns a list of results.

[jfhbrook]

So there are a few other changes in this PR - the addition of linting directions, the addition of dry run capabilities to existing commands, a fix for a shadowed variable and some updates to the pester tests. Are you not interested in these? I can rework this PR to remove the alias. I can even break it up into more than one pull request. I'm happy to do this. The alias is just the thing that caught my interest, not the raison d'etre of this PR.

Anyway: I agree that the naming convention is a little weird, but I would point out that the way the PS cmdlets work is usually that they take filters and then return any amount of results. It's relatively rare for there to be a semantic difference between grabbing more than one result, versus doing the same but with a filter that just happens to return a single result. Think SQL here - SQL always gives you a collection of rows as a result but that result might have one element in it. This has ramifactions for #81 as well - Getting the current node version is the same as querying all node versions but with a filter that's effectively where active = true. This is of course your project and you can buck PowerShell conventions if you want to, but I think reframing the semantics to follow this SQL-y pattern might be the way you'd square the circle. Just some food for thought.

[aaronpowell]

Happy for you to spin the linting and dry run stuff into a separate PR.

[jfhbrook]

OK to reuse this one? I'll rebase the commits

[aaronpowell]

I understand what the convention is trying to achieve for PowerShell but I ultimately don't feel it fits with the expectation of how this commandlet would work. Since the module doesn't do anything to track the "current active version", all it can do is node --version, and then the commandlet is providing no value.

But say hypothetically it did work that way and when you run Get-NodeVersion returns the current version, what does -Filter do? Does it filter on the current version to see if it matches the current version? Do we need an -All parameter to clearly indicate that you're wanting all available or do you flip it to always return a list and if you provide -Active it returns node --version.

Fundamentally, I feel like it's a breaking change renaming the commandlet as it changes the semantics of use and would require a rethink of how to use that function and what the expectations are.

[aaronpowell]

Spike a new one, that way this conversation can represents a point-in-time reference as to why the API is the way it is.

[jfhbrook]

Agree that changing the semantics of the cmdlet would be a breaking change.

But hypothetically: versions in ps-nvm would effectively be a table or view with various fields on them. Something like this:

Version	Tag	Installed	Available	Active	Url	Path	Bin
v14.3.0	Current	False	True	False	https://nodejs.org/dist/v14.3.0/node-v14.3.0-win-x64.zip	null	null
v12.17.0	LTS	True	True	True	https://nodejs.org/dist/v14.3.0/node-v14.3.0-win-x64.zip	/some/path/to/install	/some/path/to/install/bin/node

Then when querying that table with a hypothetical Get-NodeVersion you end up putting filters on what you return from that table with things like -Available or -Installed or -Tag Current and you would get PSObjects in return - so you could run something like, (Get-NodeVersion -Active).Version to get the currently installed node version, or Get-NodeVersion -Available -Version "~12.7" to get patch versions available for 12.7.

I'm not going to argue that you should rewrite your library to support this API of course - this sounds like a lot of work - but an API like this would be PowerShell-y.

[jfhbrook]

Anyway - I'll send you that PR in the morning. :+1:

[aaronpowell]

Yeah, I was starting to think it through more and I have a feeling that internally the module needs to track what version of node is active, instead of relying on the Windows PATH directly. Then there's more "smart" things that could be done, such as your suggested table.