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 — Fix profile collection on non-Windows, add PS 7 profiles by rjmholt · Pull Request #1260 · PowerShell/PSScriptAnalyzer

Conversation

@rjmholt

@rjmholt Rob Holt (rjmholt) commented Jun 14, 2019

Copy link
Copy Markdown
Contributor

PR Summary

  • Fixes some issues with profile collection in PS 7
  • Makes the OS Name field more descriptive and adds a Description field to capture the current value
  • Adds profiles for PS 7-preview.1 on Windows 10, Windows Server 2019 and Ubuntu 18.04.2

Services PowerShell/PowerShell#9831.

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
Robert Holt added 3 commits June 14, 2019 10:30
Robert Holt
Fix platform information collection on *nix
f0c05f1
Robert Holt
Add profiles for PS 7-preview.1
7354b6f
Robert Holt
Use correct schema version
bb2cf28
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jun 14, 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.

Some minor comments, looks OK overall though. Please wait with the merge until development is merged into master for 1.18.1. Even after that we are thinking of changing the default branch to master, so just hold off from merging atm please

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rjmholt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread ...ollector/Microsoft.PowerShell.CrossCompatibility/Collection/CompatibilityProfileCollector.cs
}

private static readonly Version s_currentProfileSchemaVersion = new Version(1, 1);
private static readonly Version s_currentProfileSchemaVersion = new Version(1, 2);

@bergmeister Christoph Bergmeister (bergmeister) Jun 14, 2019

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.

I am guessing the code works in such a way that and old schema (change only in minor version) is backwards compatible, i.e. old profiles still work?

Sorry, something went wrong.

Uh oh!

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

@rjmholt Rob Holt (rjmholt) Jun 14, 2019

Copy link
Copy Markdown
Contributor Author

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.

Exactly

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@rjmholt Rob Holt (rjmholt) Jun 14, 2019

Copy link
Copy Markdown
Contributor Author

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.

I've added a comment explaining this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...Collector/Microsoft.PowerShell.CrossCompatibility/Collection/PlatformInformationCollector.cs
{
try
{
using (FileStream fileStream = File.OpenRead(path))

@bergmeister Christoph Bergmeister (bergmeister) Jun 14, 2019

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.

DRY:

Suggested change
using (FileStream fileStream = File.OpenRead(path))
using (var fileStream = File.OpenRead(path))

Sorry, something went wrong.

Uh oh!

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

@rjmholt Rob Holt (rjmholt) Jun 14, 2019

Copy link
Copy Markdown
Contributor Author

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.

There's no repetition here; FileStream only occurs once on the line and it's not obvious that File.OpenRead returns that type

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister reacted with eyes emoji
All reactions
  • 👀 1 reaction
Comment thread ...Collector/Microsoft.PowerShell.CrossCompatibility/Collection/PlatformInformationCollector.cs Outdated
/// <returns>A dictionary with the keys and values of all the release info files on the machine.</returns>
public static IReadOnlyDictionary<string, string> GetLinuxReleaseInfo()
{
var dict = new Dictionary<string, string>();

@bergmeister Christoph Bergmeister (bergmeister) Jun 14, 2019

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.

Maybe a more descriptive name could be useful? What about creating it in a case insensitive way?

Sorry, something went wrong.

Uh oh!

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

@rjmholt Rob Holt (rjmholt) Jun 14, 2019

Copy link
Copy Markdown
Contributor Author

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.

A more descriptive name for the method? Not sure what to call it other than this, but open to suggestions.

I thought about case-sensitivity, but ultimately this is Linux-specific and there's nothing to stop two keys being added that differ only by case

Sorry, something went wrong.

Uh oh!

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

@bergmeister Christoph Bergmeister (bergmeister) Jun 14, 2019

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.

No, for dict

Sorry, something went wrong.

Uh oh!

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

@rjmholt Rob Holt (rjmholt) Jun 14, 2019

Copy link
Copy Markdown
Contributor Author

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.

Oh! Got it :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister reacted with laugh emoji
All reactions
  • 😄 1 reaction
Comment thread ...Collector/Microsoft.PowerShell.CrossCompatibility/Collection/PlatformInformationCollector.cs
}
}
}
catch (IOException)

@bergmeister Christoph Bergmeister (bergmeister) Jun 14, 2019

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.

Maybe a comment why this could happen and is OK could be helpful.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rjmholt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bergmeister
Christoph Bergmeister (bergmeister) requested a review from James Truher (JamesWTruher) June 14, 2019 20:24
@rjmholt

Rob Holt (rjmholt) commented Jun 14, 2019

Copy link
Copy Markdown
Contributor Author

Many of the changes I think here are me moving methods around, might be worth turning off whitespace changes for the diff
bergmeister 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.
@rjmholt
Address @bergmeister's feedback
4b0e141
@rjmholt
Rob Holt (rjmholt) force-pushed the ps7-profiles branch from ba31311 to 4b0e141 Compare June 14, 2019 20:38
@rjmholt
Improve dict name
5a42695
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Jun 18, 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.

LGTM but please wait with the merge until development is merged into master

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rjmholt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bergmeister
Christoph Bergmeister (bergmeister) changed the base branch from development to master June 18, 2019 21:10
@bergmeister

Christoph Bergmeister (bergmeister) commented Jun 18, 2019

Copy link
Copy Markdown
Collaborator

development is now merged into master and I retargeted the PR for master
All reactions

Sorry, something went wrong.

Uh oh!

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

Christoph Bergmeister (bergmeister) commented Aug 22, 2019

Copy link
Copy Markdown
Collaborator

Rob Holt (@rjmholt) can we merge this now?
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.
@rjmholt
Rob Holt (rjmholt) merged commit a60ee23 into PowerShell:master Aug 23, 2019
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
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher

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.

2 participants

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