👋

Looks useful, thanks!

The naming of this rule is a little awkward. It's listed as InvalidVersionConstruction in the markdown doc, so I guess you renamed it at some point? Naming things is hard! Perhaps something like InvalidDottedValue or InvalidDottedLiteral as a suggestion? I don't know though 🤷 - I'm bad at naming things - I just know the current name looks off.

This rule has a severity of Warning but is emitting a diagnostic of Error severity. Where this is the only diagnostic that your rule emits, these should match. Not sure which is more appropriate here - I can't think of a case where you'd want the unquoted dotted literal intentionally so maybe Error is correct?

In your tests, you have 2 Context blocks with the same Supressed name.

You are using the temp folder in some of your testing; I'd suggest looking into and using Pester's Test Drive for temporary file usage during tests. It helpfully takes care of relevant cleanup for you too at the right time.

I don't think the CompilerServices import is intended?

This has the same copy-pasta as your other PRs.

I am trying to be precise but it is amazing how many issues you still find in PRs.
It is the end of the day for me, but I will pick them up tomorrow and respond/act accordingly.
Again, thanks for your feedback!

Liam Peters (@liamjpeters),

It's listed as InvalidVersionConstruction in the markdown doc

Fixed, although I have kept the questionable name InvalidMultiDotValue name (as I would like to avoid that I forget to update a reference again) until somebody comes up with a accurate name that better covers the scope.

This rule has a severity of Warning but is emitting a diagnostic of Error severity

Fixed

you have 2 Context blocks with the same Suppressed name.

Fixed

I don't think the CompilerServices import is intended?

Removed

This has the same copy-pasta as #2178.

Changed accordingly

Andy Jordan (@andyleejordan)

Thank you for your feedback,

Opt-in by default. Same point as the other rules: this is currently IScriptRule, which means enabled-by-default. Even though this is a clear error case (invalid construction that produces $null), our convention for new rules is opt-in via ConfigurableRule with Enable = false, so users get to evaluate impact on existing codebases before flipping it on.

I will change the rule and derive it from ConfigurableRule once I have a better understanding of the value writing a new rule if it is disabled by default (see also: #2178 (comment)). For this particular rule, I really don't see any value in leaving an unquoted "MultiDotValue" (which results in$null) unnoticed.

The if (invalidAsts != null) guard around the foreach in Rules/InvalidMultiDotValue.cs is dead code.

Thanks, I missed that (too much PowerShell ballast and just starting with C# 😃)
I will take this up when implementing the ConfigurableRule (either disabled or enabled by default)

Open question — Liam suggested TestDrive instead of the manual temp folder in

This is already implemented:
https://github.com/PowerShell/PSScriptAnalyzer/pull/2180/changes#diff-5490ff017eca590e1028ed152db08c369f1cb97f03f106d162169fb4dd215c0cR151

I will change the rule and derive it from ConfigurableRule once I have a better understanding of the value writing a new rule if it is disabled by default (see also: #2178 (comment)). For this particular rule, I really don't see any value in leaving an unquoted "MultiDotValue" (which results in$null) unnoticed.

The problem is that, for better or worse, a lot of people have enabled PSSA with default rules set to on in their CI systems, and so if new rules show up with warnings their builds fail and we get yelled at. Should they have done that? No. But it does mean that for as long as we're doing minor releases (which get you a new package with your rules usable) then the net effect of taking said minor release should be no new warnings/errors. When we do a major release we get to break what's become the equivalent of an API promise and go and enable by default all the great new rules.

So yes, good new rules like this will eventually land as enabled by default, but it's a much bigger project for us to go through and do that and make a major release. I'd like for that to happen, we'll just have to get it on the docket.

Thanks, I missed that (too much PowerShell ballast and just starting with C# 😃)
I will take this up when implementing the ConfigurableRule (either disabled or enabled by default)

Don't thank me, thank Claude!

This is already implemented:
https://github.com/PowerShell/PSScriptAnalyzer/pull/2180/changes#diff-5490ff017eca590e1028ed152db08c369f1cb97f03f106d162169fb4dd215c0cR151

Don't blame me, blame Claude!

🤠

Andy Jordan (@andyleejordan)

...
So yes, good new rules like this will eventually land as enabled by default, but it's a much bigger project for us to go through and do that and make a major release. I'd like for that to happen, we'll just have to get it on the docket.

Thank you for the explanation.
I will make all my (recent) rules ConfigurableRule and disable them (later this weekend)

Further thoughts:
Add a specific VSCode (something like: PSScriptAnalyzerVSCodeSettings.psd1) file to the module.
But then, there is this issue: PowerShell/vscode-powershell#4653

Further thoughts: Add a specific VSCode (something like: PSScriptAnalyzerVSCodeSettings.psd1) file to the module. But then, there is this issue: PowerShell/vscode-powershell#4653

A shameless plug - I'm trying to improve on the settings file story a little and make rules and their options more discoverable with #2176.

I don't know loads about the vscode extension side of things, but I'm interested in making adding a settings file to a project super easy.

Liam Peters (@liamjpeters),

I'm interested in making adding a settings file to a project super easy

Fyi: I did a formal request for an additional settings file for developement: #2184

Christoph Bergmeister (@bergmeister),

Looks ok but what makes it WIP?

The WIP was because of the Opt-in by default convention feedback.
The next commit will be configurable and disabled by default.