★ wanayoo — archive 1999 https://github.com/PowerShell/PSScriptAnalyzer/pull/1331Nouvelle recherche | Portail wanayoo
Skip to content

/ PSScriptAnalyzer

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
Merged

Add PS 7 to UseCompatibleSyntax #1331

merged 2 commits into from Oct 21, 2019

Conversation

@rjmholt
Copy link
Member

rjmholt commented Sep 10, 2019

PR Summary

Adds PS 7 recognition to UseCompatibleSyntax

PR Checklist
@rjmholt
Add PS 7 to UseCompatibleSyntax
Loading status checks…
d44244a
@bergmeister
bergmeister approved these changes Sep 10, 2019
View changes
Copy link
Collaborator

bergmeister left a comment

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
@rjmholt @bergmeister
Update Rules/CompatibilityRules/UseCompatibleSyntax.cs
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
38c8b8c 
Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
@rjmholt
Copy link
Member Author

rjmholt commented Sep 10, 2019 •
edited

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)
@bergmeister
bergmeister approved these changes Sep 15, 2019
View changes
Copy link
Collaborator

bergmeister left a comment

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 approved these changes Oct 21, 2019
View changes
@JamesWTruher JamesWTruher merged commit 1202f4e into PowerShell:master Oct 21, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
@bergmeister bergmeister mentioned this pull request Oct 23, 2019
Merged
6 of 6 tasks complete
@bergmeister
Copy link
Collaborator

bergmeister commented Oct 26, 2019 •
edited

@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
Christoph Bergmeister
Revert "Add PS 7 to UseCompatibleSyntax (PowerShell#1331)"
Loading status checks…
c2d90e7 
This reverts commit 1202f4e.
@rjmholt
Copy link
Member 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 •
edited

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
Member 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.
@bergmeister bergmeister mentioned this pull request May 22, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bergmeister bergmeister

@JamesWTruher JamesWTruher

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@rjmholt @bergmeister @JamesWTruher
You can’t perform that action at this time.