PhysicalDiskInfo class #455 #460
Conversation
-Update unit tests;
Code improvement, work in progress.
-Removed unused references;
-Fixed broken unit test;
… NetworkInterface property.
# Conflicts: # AlphaFS/Filesystem/Directory Class/Directory.EnumerateDirectories.cs # AlphaFS/Filesystem/Directory Class/Directory.EnumerateFileIdBothDirectoryInfo.cs # AlphaFS/Filesystem/Directory Class/Directory.EnumerateFileSystemEntries.cs # AlphaFS/Filesystem/Directory Class/Directory.EnumerateFileSystemEntryInfos.cs # AlphaFS/Filesystem/Directory Class/Directory.EnumerateFiles.cs
-Moved some files around, code improvement, work in progress;
There was a problem hiding this comment.
There is really no way I can do a review of this, with 332 changed files and over 250+ commits, and no assicated issues or any hint as to what is really done here?
Seems that most classes have been split into files per method, which is fine, but makes it impossible to track down what has changed.
I know that sometime major organizational changes like this is needed, but it should be kept in separate pull-requests/branches from any feature additions so that it is possible to actually review and follow what has been done.
Can you give me some description of the changes that are actually made and what, if anything, you want me to look closer at?
| [SecurityCritical] | ||
| private object GetDeviceInfo(int type, int mode) | ||
| { | ||
| //try |
There was a problem hiding this comment.
Remove commented try/catch or use it.
| [SuppressMessage("Microsoft.Maintainability", "CA1502:AvoidExcessiveComplexity")] | ||
| [SuppressMessage("Microsoft.Design", "CA1031:DoNotCatchGeneralExceptionTypes")] | ||
| [SecurityCritical] | ||
| private object GetDeviceInfo(int type, int mode) |
There was a problem hiding this comment.
Can we change type/mode to private enums to make it clear what this method does, it is very confusing with the integers. Or at the very least, document it.
|
Hm yeah, perhaps a bit too much for a simple "review", and perhaps not the best word for it. The majority of the commits are all about the Device.PhysicalDiskInfo class and related members, So the review is not about comparing existing base code that has been improved, PhysicalDiskInfo role is to kind of act like a DiskInfo, but aimed at, well, physical disks. |
|
Alright, I'll have another look at the |
There was a problem hiding this comment.
Don't forget to update CHANGELOG.
Added a bunch of comments to various things that caught my attention. But it is really, really difficult to review a changeset of this size.
In the future, please keep refactorings (such as splitting stuff into many new files) as separate commits/pull requests from any actual changes to code, and please request code-reviews more often on smaller changes, rather than something as big as this, where it's impossible to get an overview of what has actually changed in the public surface area.
In short though the PhysicalDiskInfo seems like a good adition, though the name of the associated class Local must be changed to something more descriptive.
|
|
||
| #region Constructors | ||
|
|
||
| private PhysicalDiskInfo() |
There was a problem hiding this comment.
Constructor appears unused. Remove.
|
|
||
|
|
||
| /// <summary>[AlphaFS] Initializes an instance from a physical disk device number.</summary> | ||
| /// <param name="deviceNumber">A device number that indicates a physical disk on the Computer.</param> |
There was a problem hiding this comment.
Can we add a bit more description on what a "device number" actually is? Is it like the comment on DISK_EXTENT.DiskNumber`?
| #region Properties | ||
|
|
||
| /// <summary>An initialized <see cref="Filesystem.DeviceInfo"/> instance.</summary> | ||
| private DeviceInfo DeviceInfo { get; set; } |
There was a problem hiding this comment.
Shouldn't this be private set ?
| /// <summary>The device description.</summary> | ||
| public string DeviceDescription | ||
| { | ||
| get { return null != DeviceInfo ? DeviceInfo.DeviceDescription : string.Empty; } |
There was a problem hiding this comment.
These constructs could be abbreviated to eg. DeviceInfo?.DeviceDescription ?? String.Empty
| public sealed partial class PhysicalDiskInfo | ||
| { | ||
| /// <summary>Checks if the volume/logical drive is located on the physical disk. | ||
| /// <para>A drive path such as: <c>C</c>, <c>C:</c> or <c>C:\</c></para> |
There was a problem hiding this comment.
The two paragraphs here seems to belong with the description of the devicePath parameter (where the also appear to be. Remove them from the summary.
| /// <summary>[AlphaFS] Provides access to information of a physical disk on the Computer.</summary> | ||
| [Serializable] | ||
| [SecurityCritical] | ||
| public sealed partial class PhysicalDiskInfo |
There was a problem hiding this comment.
Can't find any updates to the CHANGELOG? I'm assuming this is a new public class?
| namespace Alphaleonis.Win32.Device | ||
| { | ||
| /// <summary>[AlphaFS] Provides static methods to retrieve device resource information from the local host.</summary> | ||
| public static partial class Local |
There was a problem hiding this comment.
The name Local is way to generic, and doesn't really say anything about what the class does. A name like Device would probably be better? Especially since it apparently can retrieve things from a remote host as well?
| [SecurityCritical] | ||
| public static IEnumerable<DeviceInfo> EnumerateDevices(DeviceGuid[] deviceGuid) | ||
| { | ||
| return EnumerateDevicesCore(null, deviceGuid, true); |
There was a problem hiding this comment.
Could we make the input array a params array instead and hence remove the previous method?
| /// <param name="hostName">The name of the local or remote host on which the device resides. <c>null</c> refers to the local host.</param> | ||
| /// <param name="deviceGuid">One or more <see cref="DeviceGuid"/> device guids.</param> | ||
| [SecurityCritical] | ||
| public static IEnumerable<DeviceInfo> EnumerateDevices(string hostName, DeviceGuid[] deviceGuid) |
| /// </remarks> | ||
| /// <param name="hostName">The name of the local or remote host on which the device resides. <c>null</c> refers to the local host.</param> | ||
| /// <param name="deviceGuid">One or more <see cref="DeviceGuid"/> device guids.</param> | ||
| /// <param name="getAllProperties"><c>true</c> to retrieve all device properties.</param> |
There was a problem hiding this comment.
What does getAllProperties mean? What is retrieved otherwise?
Hi @alphaleonis, I'd appreciate a review on this one, for a v2.3.
I think it's ready to go.