V9.0.0/remove unit prefix multiple #97
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request involves the extensive removal of several classes, interfaces, enums, and methods from the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/Cuemon.Core/Diagnostics/ProcessInfo.cs (1)
33-35: Consider improving readability for large memory valuesThe changes align with the PR objectives by removing the dependency on the
ByteStorageCapacityclass. UsingCultureInfo.InvariantCultureensures consistent formatting across different systems. However, for large memory values, the readability might be affected without unit prefixes (e.g., MB, GB).To improve readability while maintaining the simplification, consider using a custom extension method or helper function to format large numbers. For example:
public static string FormatBytes(long bytes) { string[] suffixes = { "B", "KB", "MB", "GB", "TB", "PB" }; int counter = 0; decimal number = bytes; while (Math.Round(number / 1024) >= 1) { number = number / 1024; counter++; } return string.Format(CultureInfo.InvariantCulture, "{0:n1} {1}", number, suffixes[counter]); }Then use it in the
ToStringmethod:builder.Append(FormattableString.Invariant($"WorkingSet64: {FormatBytes(Process.WorkingSet64)}")); builder.Append(Alphanumeric.CaretChar); builder.Append(FormattableString.Invariant($"TotalWorkingSet64: {FormatBytes(Process.GetProcesses().Select(p => p.WorkingSet64).Sum())}"));This approach maintains the removal of the
ByteStorageCapacitydependency while improving the readability of the output.src/Cuemon.Security.Cryptography/AesCryptor.cs (1)
Line range hint
127-131: LGTM! Consider minor optimization.The changes look good and are consistent with the refactoring effort. The functionality of the
GenerateInitializationVectormethod remains unchanged.Consider a minor optimization to improve readability and reduce repeated calculations:
- return Convertible.GetBytes(Generate.RandomString(BlockSize / Convertible.BitsPerByte, Alphanumeric.LettersAndNumbers, Alphanumeric.PunctuationMarks), options => + const int initVectorLength = BlockSize / Convertible.BitsPerByte; + return Convertible.GetBytes(Generate.RandomString(initVectorLength, Alphanumeric.LettersAndNumbers, Alphanumeric.PunctuationMarks), options =>This change introduces a constant for the initialization vector length, which improves readability and ensures the calculation is only done once.
src/Cuemon.Extensions.Net/Security/SignedUriOptions.cs (1)
179-180: LGTM! Consider updating error messages for clarity.The changes correctly replace
ByteUnit.BitsPerNibblewithConvertible.BitsPerNibble, aligning with the PR objectives of removing the units of measurement system. The validation logic remains correct, ensuring theContentMd5Headervalue is exactly 32 bytes (128 bits) long, which is appropriate for an MD5 hash.Consider updating the error messages for improved clarity:
- Validator.ThrowIfLowerThan(value.Length, MessageDigest5.BitSize / Convertible.BitsPerNibble, nameof(value), FormattableString.Invariant($"The length of the value is lower than 128-bit / 32 byte.")); - Validator.ThrowIfGreaterThan(value.Length, MessageDigest5.BitSize / Convertible.BitsPerNibble, nameof(value), FormattableString.Invariant($"The length of the value is greater than 128-bit / 32 byte.")); + Validator.ThrowIfLowerThan(value.Length, MessageDigest5.BitSize / Convertible.BitsPerNibble, nameof(value), FormattableString.Invariant($"The length of the value must be exactly 32 bytes (128 bits).")); + Validator.ThrowIfGreaterThan(value.Length, MessageDigest5.BitSize / Convertible.BitsPerNibble, nameof(value), FormattableString.Invariant($"The length of the value must be exactly 32 bytes (128 bits)."));This change would make the error messages more precise and consistent with the actual validation being performed.
src/Cuemon.Core/Convertible.cs (1)
112-112: LGTM: Simplified bit size calculationThe modification to use the newly added
BitsPerByteconstant instead ofByteUnit.BitsPerBytealigns well with the PR objective of removing the units of measurement system. This change reduces dependencies and simplifies the code while maintaining the correct functionality.Consider renaming the
bitSizevariable tototalBitsfor improved clarity:-var bitSize = byteSize * BitsPerByte; +var totalBits = byteSize * BitsPerByte;This small change could make the code even more self-explanatory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (31)
- src/Cuemon.Core/BinaryPrefix.cs (0 hunks)
- src/Cuemon.Core/BitStorageCapacity.cs (0 hunks)
- src/Cuemon.Core/BitUnit.cs (0 hunks)
- src/Cuemon.Core/ByteStorageCapacity.cs (0 hunks)
- src/Cuemon.Core/ByteUnit.cs (0 hunks)
- src/Cuemon.Core/Convertible.cs (3 hunks)
- src/Cuemon.Core/DecimalPrefix.cs (0 hunks)
- src/Cuemon.Core/Diagnostics/ProcessInfo.cs (1 hunks)
- src/Cuemon.Core/Extensions/PrefixMultipleDecoratorExtensions.cs (0 hunks)
- src/Cuemon.Core/IPrefixMultiple.cs (0 hunks)
- src/Cuemon.Core/IUnit.cs (0 hunks)
- src/Cuemon.Core/MultipleTable.cs (0 hunks)
- src/Cuemon.Core/NamingStyle.cs (0 hunks)
- src/Cuemon.Core/PrefixMultiple.cs (0 hunks)
- src/Cuemon.Core/PrefixUnit.cs (0 hunks)
- src/Cuemon.Core/Security/FowlerNollVoHash.cs (1 hunks)
- src/Cuemon.Core/StorageCapacity.cs (0 hunks)
- src/Cuemon.Core/StorageCapacityOptions.cs (0 hunks)
- src/Cuemon.Core/UnitFormatOptions.cs (0 hunks)
- src/Cuemon.Core/UnitPrefix.cs (0 hunks)
- src/Cuemon.Core/UnitPrefixFormatter.cs (0 hunks)
- src/Cuemon.Core/ZeroPrefix.cs (0 hunks)
- src/Cuemon.Extensions.Net/Security/SignedUriOptions.cs (1 hunks)
- src/Cuemon.Security.Cryptography/AesCryptor.cs (2 hunks)
- src/Cuemon.Security.Cryptography/AesKeyOptions.cs (1 hunks)
- test/Cuemon.Core.Tests/BinaryPrefixTest.cs (0 hunks)
- test/Cuemon.Core.Tests/BitStorageCapacityTest.cs (0 hunks)
- test/Cuemon.Core.Tests/ByteStorageCapacityTest.cs (0 hunks)
- test/Cuemon.Core.Tests/DecimalPrefixTest.cs (0 hunks)
- test/Cuemon.Extensions.IO.Tests/StreamExtensionsTest.cs (6 hunks)
- test/Cuemon.IO.Tests/StreamDecoratorExtensionsTest.cs (6 hunks)
💤 Files with no reviewable changes (23)
- src/Cuemon.Core/BinaryPrefix.cs
- src/Cuemon.Core/BitStorageCapacity.cs
- src/Cuemon.Core/BitUnit.cs
- src/Cuemon.Core/ByteStorageCapacity.cs
- src/Cuemon.Core/ByteUnit.cs
- src/Cuemon.Core/DecimalPrefix.cs
- src/Cuemon.Core/Extensions/PrefixMultipleDecoratorExtensions.cs
- src/Cuemon.Core/IPrefixMultiple.cs
- src/Cuemon.Core/IUnit.cs
- src/Cuemon.Core/MultipleTable.cs
- src/Cuemon.Core/NamingStyle.cs
- src/Cuemon.Core/PrefixMultiple.cs
- src/Cuemon.Core/PrefixUnit.cs
- src/Cuemon.Core/StorageCapacity.cs
- src/Cuemon.Core/StorageCapacityOptions.cs
- src/Cuemon.Core/UnitFormatOptions.cs
- src/Cuemon.Core/UnitPrefix.cs
- src/Cuemon.Core/UnitPrefixFormatter.cs
- src/Cuemon.Core/ZeroPrefix.cs
- test/Cuemon.Core.Tests/BinaryPrefixTest.cs
- test/Cuemon.Core.Tests/BitStorageCapacityTest.cs
- test/Cuemon.Core.Tests/ByteStorageCapacityTest.cs
- test/Cuemon.Core.Tests/DecimalPrefixTest.cs
✅ Files skipped from review due to trivial changes (1)
- test/Cuemon.Extensions.IO.Tests/StreamExtensionsTest.cs
🧰 Additional context used
🔇 Additional comments (12)
src/Cuemon.Core/Security/FowlerNollVoHash.cs (1)
69-69: LGTM: Change aligns with PR objectivesThe replacement of
ByteUnit.BitsPerBytewithConvertible.BitsPerByteis consistent with the PR's goal of removing the units of measurement system from the codebase. This change should maintain the same functionality while simplifying the code structure.To ensure this change is consistent across the codebase, let's verify the usage of
Convertible.BitsPerByte:✅ Verification successful
All active code now uses
Convertible.BitsPerByte, and there are no remaining instances ofByteUnit.BitsPerByteoutside of commented sections. The change aligns with the PR objectives of removing the units of measurement system from the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of Convertible.BitsPerByte and check for any remaining ByteUnit.BitsPerByte # Test 1: Search for Convertible.BitsPerByte usage echo "Occurrences of Convertible.BitsPerByte:" rg --type csharp "Convertible\.BitsPerByte" # Test 2: Check if there are any remaining instances of ByteUnit.BitsPerByte echo "Checking for any remaining ByteUnit.BitsPerByte:" rg --type csharp "ByteUnit\.BitsPerByte"Length of output: 1746
src/Cuemon.Security.Cryptography/AesKeyOptions.cs (1)
62-66: LGTM! Consistent replacement of ByteUnit with Convertible.The changes from
ByteUnit.BitsPerBytetoConvertible.BitsPerByteare consistent and align with the PR objectives of removing the units of measurement system. The functionality remains unchanged, asBitsPerByteis likely a constant value (8) in both cases.To ensure consistency across the codebase, let's verify the usage of
Convertible.BitsPerByte:✅ Verification successful
All usages correctly replaced with Convertible.BitsPerByte
Verified that all active code now uses
Convertible.BitsPerByte. Any remaining instances ofByteUnit.BitsPerByteare confined to commented-out sections and do not affect the functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of Convertible.BitsPerByte and check for any remaining ByteUnit.BitsPerByte # Test 1: Search for Convertible.BitsPerByte usage echo "Occurrences of Convertible.BitsPerByte:" rg --type csharp "Convertible\.BitsPerByte" # Test 2: Search for any remaining ByteUnit.BitsPerByte echo "\nChecking for any remaining ByteUnit.BitsPerByte:" rg --type csharp "ByteUnit\.BitsPerByte"Length of output: 1750
src/Cuemon.Security.Cryptography/AesCryptor.cs (2)
Line range hint
1-156: Overall assessment: Changes align with PR objectivesThe modifications in this file successfully implement the removal of the units of measurement system as per the PR objectives. The functionality of the
AesCryptorclass remains intact, with only the source of constants changing fromByteUnittoConvertible. These changes contribute to the overall goal of simplifying the codebase and improving maintainability.
43-44: LGTM! Verify constant in Convertible class.The changes look good and align with the PR objectives of removing the units of measurement system. The functionality remains the same, only the source of the constant has changed from
ByteUnit.BitsPerBytetoConvertible.BitsPerByte.To ensure the constant exists in the
Convertibleclass, please run the following script:test/Cuemon.IO.Tests/StreamDecoratorExtensionsTest.cs (7)
41-43: LGTM: Output simplified while preserving essential information.The changes to the output statements simplify the code by directly using stream lengths instead of
ByteStorageCapacity.FromBytes(). This modification aligns with the PR objective of removing the units of measurement system while still providing the necessary information about stream sizes.
65-67: LGTM: Consistent simplification of output in async method.The changes to the output statements in this async method mirror those made in the synchronous version, maintaining consistency across the codebase. The direct use of stream lengths instead of
ByteStorageCapacity.FromBytes()aligns with the PR objective of removing the units of measurement system.
105-107: LGTM: Consistent simplification across compression methods.The changes to the output statements in the GZip compression method are consistent with those made in the Brotli compression methods. This uniform application of simplification across different compression algorithms maintains code consistency and aligns with the PR objective of removing the units of measurement system.
129-131: LGTM: Consistent simplification in async GZip method.The changes to the output statements in the async GZip compression method are consistent with those made in the synchronous version and other compression methods. This uniform application of simplification across both synchronous and asynchronous methods maintains code consistency and continues to align with the PR objective of removing the units of measurement system.
167-169: LGTM: Consistent simplification across all compression methods.The changes to the output statements in the Deflate compression method are consistent with those made in the Brotli and GZip compression methods. This uniform application of simplification across all compression algorithms maintains code consistency and continues to align with the PR objective of removing the units of measurement system.
191-193: LGTM: Consistent simplification across all methods and compression algorithms.The changes to the output statements in the async Deflate compression method are consistent with those made in all other compression methods, both synchronous and asynchronous. This uniform application of simplification across all methods and compression algorithms maintains excellent code consistency and fully aligns with the PR objective of removing the units of measurement system.
41-43: Overall assessment: Consistent and beneficial simplification across the file.The changes made throughout this file demonstrate a consistent approach to simplifying the output statements in all compression test methods, both synchronous and asynchronous. By removing the
ByteStorageCapacity.FromBytes()calls and directly using stream lengths, the code becomes more straightforward while still providing the necessary information about stream sizes.These modifications align perfectly with the PR objective of removing the units of measurement system from the codebase. The consistency of these changes across all methods and compression algorithms (Brotli, GZip, and Deflate) is commendable, as it maintains code uniformity and improves overall readability.
The simplification does not appear to impact the functionality or effectiveness of the tests, as the essential information about stream lengths is still preserved in the output.
Also applies to: 65-67, 105-107, 129-131, 167-169, 191-193
src/Cuemon.Core/Convertible.cs (1)
46-54: LGTM: Well-defined constants for bit operationsThe addition of
BitsPerByteandBitsPerNibbleconstants is a good practice. They provide clear, meaningful values that can be used throughout the codebase, improving readability and maintainability. The calculation ofBitsPerNibblebased onBitsPerByteensures consistency if the value ofBitsPerByteever needs to change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/Cuemon.Extensions.Core/RoundOffAccuracy.cs (1)
Line range hint
7-33: Consider future extensibility for higher precision.The current enum covers a good range of precisions up to "NearestMillion". However, if there's a possibility that even higher precision might be needed in the future (e.g., NearestBillion, NearestTrillion), it might be worth considering how to extend this enum or make it more flexible.
Options to consider:
- Add higher precision values if they're likely to be needed.
- Implement a more dynamic system for precision that doesn't rely on a fixed enum, if extremely high or variable precision is required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/Cuemon.Extensions.Core/DoubleExtensions.cs (1 hunks)
- src/Cuemon.Extensions.Core/RoundOffAccuracy.cs (2 hunks)
- src/Cuemon.Threading/RelationalOperator.cs (2 hunks)
- test/Cuemon.Extensions.Core.Tests/DoubleExtensionsTest.cs (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Cuemon.Extensions.Core/DoubleExtensions.cs
- test/Cuemon.Extensions.Core.Tests/DoubleExtensionsTest.cs
🧰 Additional context used
🔇 Additional comments (3)
src/Cuemon.Threading/RelationalOperator.cs (2)
Line range hint
2-36: Well-structured and documented enum.The
RelationalOperatorenum is well-defined and thoroughly documented:
- Each member has a clear, descriptive name and helpful XML documentation.
- The enum includes a general description and a reference to external documentation.
- The public accessibility is appropriate for its likely usage across the codebase.
This structure promotes code readability and maintainability.
1-1: Namespace change approved.The namespace change from
CuemontoCuemon.Threadingaligns with the PR objectives of code cleanup and refactoring. This change appropriately categorizes theRelationalOperatorenum under threading-related functionalities.Let's verify that there are no remaining usages of the old namespace:
src/Cuemon.Extensions.Core/RoundOffAccuracy.cs (1)
Line range hint
2-33: LGTM: Enum contents remain unchanged.The
RoundOffAccuracyenum members and their values are correctly maintained. The XML documentation for each member is clear and concise, providing good context for developers using this enum.
| @@ -1,4 +1,4 @@ | |||
| namespace Cuemon | |||
| namespace Cuemon.Extensions | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration guide missing and lingering namespace references.
The MigrationGuide.md file is missing, which means the namespace change from Cuemon to Cuemon.Extensions isn't documented. Additionally, there are still several using Cuemon. statements and comments referencing the old namespace throughout the codebase. This could lead to confusion and potential maintenance issues.
🔗 Analysis chain
Verify the impact of the namespace change.
The namespace has been changed from Cuemon to Cuemon.Extensions. While this change may improve code organization, it could potentially break existing code that references the RoundOffAccuracy enum.
Please ensure that:
- This change is consistent across the codebase.
- It's documented in the migration guide for v9.0.0.
- All references to this enum are updated accordingly.
Run the following script to verify the usage of RoundOffAccuracy in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to RoundOffAccuracy in the old namespace
# Test 1: Search for RoundOffAccuracy usage in the old namespace
echo "Searching for RoundOffAccuracy in the old namespace:"
rg "Cuemon\.RoundOffAccuracy" --type cs
# Test 2: Verify that RoundOffAccuracy is now used with the new namespace
echo "Verifying RoundOffAccuracy usage in the new namespace:"
rg "Cuemon\.Extensions\.RoundOffAccuracy" --type cs
# Test 3: Check for any import statements that might need updating
echo "Checking for import statements that might need updating:"
rg "using Cuemon;" --type cs
Length of output: 483
Script:
#!/bin/bash
# Description: Verify that the migration guide includes the namespace change
# Test 1: Check if MigrationGuide.md mentions the namespace update
echo "Searching for namespace change in MigrationGuide.md:"
rg "Cuemon\.Extensions" MigrationGuide.md
# Test 2: Ensure no import statements use the old namespace outside of the specified file
echo "Searching for using Cuemon. in any .cs files except RoundOffAccuracy.cs:"
rg "using Cuemon\." --type cs --glob '!src/Cuemon.Extensions.Core/RoundOffAccuracy.cs'
# Test 3: Look for any comments referencing the old namespace
echo "Searching for comments mentioning the old namespace:"
rg "//.*Cuemon\." --type cs
Length of output: 104437
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
==========================================
- Coverage 80.14% 79.61% -0.54%
==========================================
Files 614 606 -8
Lines 18939 18618 -321
Branches 1914 1897 -17
==========================================
- Hits 15179 14823 -356
- Misses 3710 3727 +17
- Partials 50 68 +18 ☔ View full report in Codecov by Sentry. |
|
PR Classification
Code cleanup and refactoring to remove the units of measurement system.
PR Summary
Significant refactoring to remove classes and interfaces related to units of measurement, and update references to constants.
BinaryPrefix,BitStorageCapacity,ByteStorageCapacity,ByteUnit,DecimalPrefix,IPrefixMultiple,IUnit,PrefixMultipleDecoratorExtensions,BitsPerByteandBitsPerNibbletoConvertibleclass,ProcessInfoto use direct string conversion for memory sizes,BinaryPrefixTest.cs,BitStorageCapacityTest.cs,ByteStorageCapacityTest.cs,DecimalPrefixTest.cs,StreamExtensionsTest.csandStreamDecoratorExtensionsTest.csby removingByteStorageCapacity.FromBytesmethod calls.Summary by CodeRabbit
Bug Fixes
Documentation
Chores
DoubleExtensionsclass, enhancing functionality without altering public interfaces.