Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PS 7 to UseCompatibleSyntax #1331

Merged
merged 2 commits into from
Oct 21, 2019

Conversation

rjmholt
Copy link
Contributor

@rjmholt rjmholt commented Sep 10, 2019

PR Summary

Adds PS 7 recognition to UseCompatibleSyntax

PR Checklist

Copy link
Collaborator

@bergmeister bergmeister left a comment

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)

Rules/CompatibilityRules/UseCompatibleSyntax.cs Outdated Show resolved Hide resolved
Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
@rjmholt
Copy link
Contributor Author

rjmholt commented Sep 10, 2019

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)

Copy link
Collaborator

@bergmeister bergmeister left a comment

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 bergmeister reopened this Sep 15, 2019
@JamesWTruher JamesWTruher merged commit 1202f4e into PowerShell:master Oct 21, 2019
@bergmeister
Copy link
Collaborator

bergmeister commented Oct 26, 2019

@rjmholt It seems because the PR got merged a long time after the last PR build that there is an integration tests failure as there are 4 failing tests in master since this merge. Can you please open a PR with a fix for it?
EDIT: I revert this PR in a branch but the tests still fail, therefore something in the AppVeyor Ubuntu image must've changed. https://ci.appveyor.com/project/bergmeister/psscriptanalyzer-qdyb0/builds/28401552/job/nvab1gofa0bvqr02

bergmeister pushed a commit to bergmeister/PSScriptAnalyzer that referenced this pull request Oct 26, 2019
@rjmholt
Copy link
Contributor Author

rjmholt commented Nov 2, 2019

@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.

@bergmeister
Copy link
Collaborator

bergmeister commented Nov 2, 2019

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...

@rjmholt
Copy link
Contributor Author

rjmholt commented Nov 4, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants