GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add PS 7 to UseCompatibleSyntax by rjmholt · Pull Request #1331 · PowerShell/PSScriptAnalyzer

Conversation

@rjmholt

@rjmholt Rob Holt (rjmholt) commented Sep 10, 2019

Copy link
Copy Markdown
Contributor

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, .ps1 and .psm1 files 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rjmholt
Add PS 7 to UseCompatibleSyntax
d44244a
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Sep 10, 2019
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this 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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Rules/CompatibilityRules/UseCompatibleSyntax.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@rjmholt @bergmeister
Update Rules/CompatibilityRules/UseCompatibleSyntax.cs
38c8b8c 
Co-Authored-By: Christoph Bergmeister [MVP] <[email protected]>
@rjmholt

Rob Holt (rjmholt) commented Sep 10, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Sep 15, 2019
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this 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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bergmeister Christoph Bergmeister (bergmeister) closed this Sep 15, 2019
@bergmeister Christoph Bergmeister (bergmeister) reopened this Sep 15, 2019
JamesWTruher
James Truher (JamesWTruher) approved these changes Oct 21, 2019
View reviewed changes
@JamesWTruher
James Truher (JamesWTruher) merged commit 1202f4e into PowerShell:master Oct 21, 2019
@bergmeister Christoph Bergmeister (bergmeister) mentioned this pull request Oct 23, 2019
New rule: AvoidOverwritingBuiltInCmdlets #1348
Merged
6 tasks done
@bergmeister

Christoph Bergmeister (bergmeister) commented Oct 26, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

Rob Holt (@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
rjmholt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Christoph Bergmeister (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)"
c2d90e7 
This reverts commit 1202f4e.
@rjmholt

Rob Holt (rjmholt) commented Nov 2, 2019

Copy link
Copy Markdown
Contributor Author

James Truher (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bergmeister

Christoph Bergmeister (bergmeister) commented Nov 2, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rjmholt

Rob Holt (rjmholt) commented Nov 4, 2019

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bergmeister Christoph Bergmeister (bergmeister) mentioned this pull request May 22, 2020
UseCompatibleSyntax.md Documentation Out of Date #1512
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergmeister Christoph Bergmeister (bergmeister) bergmeister approved these changes
+1 more reviewer
@JamesWTruher James Truher (JamesWTruher) JamesWTruher approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@rjmholt @bergmeister @JamesWTruher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.