X Tutup
The Wayback Machine - https://web.archive.org/web/20200918095225/https://github.com/PowerShell/PSScriptAnalyzer/pull/1260
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

Fix profile collection on non-Windows, add PS 7 profiles #1260

Merged
merged 5 commits into from Aug 23, 2019

Conversation

@rjmholt
Copy link
Member

rjmholt commented Jun 14, 2019

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

Copy link
Collaborator

bergmeister left a comment

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

@@ -82,7 +82,7 @@ public CompatibilityProfileCollector Build(SMA.PowerShell pwsh)
}
}

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

This comment has been minimized.

@bergmeister

bergmeister Jun 14, 2019 Collaborator

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?

This comment has been minimized.

@rjmholt

rjmholt Jun 14, 2019 Author Member

Exactly

This comment has been minimized.

@rjmholt

rjmholt Jun 14, 2019 Author Member

I've added a comment explaining this

{
try
{
using (FileStream fileStream = File.OpenRead(path))

This comment has been minimized.

@bergmeister

bergmeister Jun 14, 2019 Collaborator

DRY:

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

This comment has been minimized.

@rjmholt

rjmholt Jun 14, 2019 Author Member

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

/// <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>();

This comment has been minimized.

@bergmeister

bergmeister Jun 14, 2019 Collaborator

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

This comment has been minimized.

@rjmholt

rjmholt Jun 14, 2019 Author Member

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

This comment has been minimized.

@bergmeister

bergmeister Jun 14, 2019 Collaborator

No, for dict

This comment has been minimized.

@rjmholt

rjmholt Jun 14, 2019 Author Member

Oh! Got it :)

}
}
}
catch (IOException)

This comment has been minimized.

@bergmeister

bergmeister Jun 14, 2019 Collaborator

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

@bergmeister bergmeister requested a review from JamesWTruher Jun 14, 2019
@rjmholt
Copy link
Member Author

rjmholt commented Jun 14, 2019

Many of the changes I think here are me moving methods around, might be worth turning off whitespace changes for the diff

@rjmholt rjmholt force-pushed the rjmholt:ps7-profiles branch from ba31311 to 4b0e141 Jun 14, 2019
Copy link
Collaborator

bergmeister left a comment

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

@bergmeister bergmeister changed the base branch from development to master Jun 18, 2019
@bergmeister
Copy link
Collaborator

bergmeister commented Jun 18, 2019

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

@bergmeister
Copy link
Collaborator

bergmeister commented Aug 22, 2019

@rjmholt can we merge this now?

@rjmholt rjmholt merged commit a60ee23 into PowerShell:master Aug 23, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.
X Tutup