Add PS 7 to UseCompatibleSyntax by rjmholt · Pull Request #1331 · PowerShell/PSScriptAnalyzer
PR Summary
Adds PS 7 recognition to UseCompatibleSyntax
PR Checklist
- PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
- Summarized changes
- Change is not breaking
- Make sure all
.cs,.ps1and.psm1files have the correct copyright header - Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
- This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix
WIP:to the beginning of the title and remove the prefix when the PR is ready.
- If the PR is work in progress, please add the prefix
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok but it would be nicer to have a helper method that allowed to specify something like a minimum version and returned an IEnumerable of version strings to avoid some hard coding as I imagine finding those places to be updated will be hard if there will be PS 8 (probably due to .net 5)
Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
Well the minimum version doesn't solve all issues; some syntaxes only work below or in a certain version of PowerShell, like workflow and parallel.
The places to update correspond exactly to a syntax, so I don't think that can be simplified, only moved around.
Perhaps we need a nicer data structure to deal with version checks (which can deal with only specifying a major version or not)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, other improvements are optional and could be done in later PRs. This PR can go ahead, I'll just close and re-open to re-trigger the build (which should be green now as we've fixed the Ubuntu build)
bergmeister pushed a commit to bergmeister/PSScriptAnalyzer that referenced this pull request
@JamesWTruher and I looked into this yesterday for a while. We weren't able to reproduce it on the 16.04 Docker image, but I feel I have before when I investigated it (I feel like I wrote it in an issue somewhere, but can't find it). Anyway, this is on my list for next week, so I'll keep trying then.
Yes, the fact that some of your tests fail on Ubuntu16 on AppVeyor now as well, really makes me think we either have to find the root cause and fix them or declare technical debt and ignore them on Linux only. AppVeyor also has an Ubuntu1804 image, unfortunately the 4 test failures happen there as well...
The problem on a higher level though is that similar tests fail on Ubuntu in Azure DevOps and are a blocker for moving to Azure DevOps. Azure DevOps would give us much faster builds due to free parallelism (Microsoft stopped paying for AppVeyor a few months ago and is on the free plan now) and also add Mac to the tested platforms. How essential are those few test cases? Having them work at least on Mac is good enough IMHO if the amount of effort seems to be an unknown black hole. Interestingly the 4 AppVeyor failures do not occur on the Azure DevOps images, but the failures are in a similar area. so somehow they are related...
Those tests aren't essential at all in my opinion; they aren't substantially different from the other tests (just added coverage) and we can't repro them. So if they're a blocker we can disable or remove them to get you unblocked while I investigate the cause.