Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix profile collection on non-Windows, add PS 7 profiles #1260
Conversation
|
Some minor comments, looks OK overall though. Please wait with the merge until |
| @@ -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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| { | ||
| try | ||
| { | ||
| using (FileStream fileStream = File.OpenRead(path)) |
This comment has been minimized.
This comment has been minimized.
bergmeister
Jun 14, 2019
Collaborator
DRY:
| using (FileStream fileStream = File.OpenRead(path)) | |
| using (var fileStream = File.OpenRead(path)) |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
| } | ||
| } | ||
| catch (IOException) |
This comment has been minimized.
This comment has been minimized.
bergmeister
Jun 14, 2019
Collaborator
Maybe a comment why this could happen and is OK could be helpful.
|
Many of the changes I think here are me moving methods around, might be worth turning off whitespace changes for the diff |
|
LGTM but please wait with the merge until development is merged into master |
|
development is now merged into master and I retargeted the PR for master |
|
@rjmholt can we merge this now? |

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

rjmholt commentedJun 14, 2019
PR Summary
Namefield more descriptive and adds aDescriptionfield to capture the current valueServices PowerShell/PowerShell#9831.
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.