chore: Add parameter for soft wrapping in ASCII table #1232
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the Changes
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
Documentation and Community
|
elsand
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs (3 hunks)
- src/Digdir.Tool.Dialogporten.SlackNotifier/Common/Extensions.cs (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/Extensions.cs (1)
14-14: Approve change and suggest improvementsThe addition of
maxColumnWidth: 72to theToAsciiTablemethod call aligns well with the PR objectives of introducing soft wrapping in ASCII tables. This change should improve readability by wrapping long text entries.However, to ensure the robustness of this new feature, consider the following suggestions:
- Add unit tests to verify the new formatting behavior with various input scenarios, including edge cases.
- Update the documentation to reflect this new feature and its usage.
- Could you provide more context on why 72 was chosen as the max column width? Is this value configurable, and does it work well for all expected use cases?
To verify the impact of this change, let's check for any other usages of
ToAsciiTablethat might need similar updates:✅ Verification successful
Consistent
maxColumnWidthUsage ConfirmedThe update to use
maxColumnWidth: 72inExtensions.csis the only change found. All other usages ofToAsciiTablecontinue to use the defaultmaxColumnWidth: 30, ensuring no unintended side effects in different parts of the codebase.Consider documenting the rationale behind choosing
72as the maximum column width to aid future maintenance and clarity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of ToAsciiTable method rg --type csharp "ToAsciiTable\(" -C 3Length of output: 2252
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Outdated
Show resolved
Hide resolved
…rmatter.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (4)
- Digdir.Domain.Dialogporten.sln (3 hunks)
- src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs (2 hunks)
- tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs (1 hunks)
- tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests.csproj (1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs (2)
Learnt from: elsand PR: digdir/dialogporten#1232 File: src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs:0-0 Timestamp: 2024-10-07T07:45:33.970Z Learning: The `ToAsciiTable` method creates a copy of the input `rows` and operates on it, so the original `rows` passed to the method remains unchanged.Learnt from: elsand PR: digdir/dialogporten#1232 File: src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs:12-12 Timestamp: 2024-10-07T07:31:46.700Z Learning: In C# 12, square brackets `[]` can be used for array initialization, e.g., `string[] array = [ "value" ];`.
🔇 Additional comments (9)
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests.csproj (3)
1-10: LGTM: Project configuration is well-structured.The project is correctly set up as a .NET 8.0 test project with appropriate settings:
- Implicit usings and nullable reference types are enabled, which are good practices.
- The project is correctly marked as non-packable and a test project.
19-21: LGTM: Global using statement for xUnit is appropriate.The global using statement for Xunit is a convenient approach, consistent with the project's use of implicit usings. This will make xUnit assertions available throughout the test project without requiring explicit imports in each file.
23-25: LGTM: Project reference to the main Slack Notifier project is included.The project reference seems correct, allowing the test project to access and test the main project's code.
To ensure the project structure is correct, please run the following script:
✅ Verification successful
Verified: Referenced project file exists.
The project reference is confirmed to be valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the referenced project file # Test: Check if the referenced project file exists if fd -p "src/Digdir.Tool.Dialogporten.SlackNotifier/Digdir.Tool.Dialogporten.SlackNotifier.csproj"; then echo "Referenced project file found." else echo "Warning: Referenced project file not found. Please verify the project structure." fiLength of output: 256
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs (2)
7-34: LGTM: Well-structured test for text wrappingThis test case effectively verifies the text wrapping functionality when the content exceeds the specified maximum column width. The arrangement, action, and assertion are clear and appropriate.
63-88: LGTM: Proper null handling testThis test case effectively verifies that null values in cells are handled gracefully. It's crucial for ensuring robustness in the
ToAsciiTablemethod when dealing with potentially null data.Digdir.Domain.Dialogporten.sln (2)
66-67: Summary of changes to the solution fileThe changes to the
Digdir.Domain.Dialogporten.slnfile are minimal and focused:
- A new test project
Digdir.Tool.Dialogporten.SlackNotifier.Testshas been added.- The project is correctly configured with Debug and Release build settings.
- It's properly nested under the "tests" section of the solution.
These changes enhance the project structure by adding dedicated tests for the SlackNotifier tool, which is a positive step towards improving code quality and maintainability.
Also applies to: 158-161, 191-191
66-67: New test project added: Digdir.Tool.Dialogporten.SlackNotifier.TestsThe addition of a new test project for the SlackNotifier tool is a positive change. It suggests an improvement in test coverage, which aligns with best practices for maintaining code quality.
A few observations:
- The project is correctly added to the "tests" section of the solution.
- Both Debug and Release configurations are properly set up for the new project.
- The project GUID is unique, which is correct.
To ensure the new test project is properly set up, please run the following script:
This script will help verify that the new test project is properly structured and references the necessary components.
Also applies to: 158-161, 191-191
✅ Verification successful
Verification Successful: Digdir.Tool.Dialogporten.SlackNotifier.Tests
The new test project has been successfully added and properly configured. The project file exists, includes the necessary test framework reference, and correctly references the main SlackNotifier project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and basic structure of the new test project # Check if the project file exists fd -p "tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests.csproj" # Check for basic test setup (e.g., reference to test framework) rg -i "Microsoft\.NET\.Test\.Sdk" "tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests.csproj" # Check for reference to the main project rg -i "Digdir\.Tool\.Dialogporten\.SlackNotifier" "tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests.csproj"Length of output: 717
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs (2)
42-42: Correctly handlenullcell valuesGreat job updating the code to safely handle
nullvalues by using the null-conditional operator?.. This prevents potentialNullReferenceExceptionwhen processing cell content.
7-10: 💡 Codebase verificationUpdate Required for
ToAsciiTableUsage in Extensions.cs
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/Extensions.cs: TheToAsciiTable()method is called without specifying themaxColumnWidthparameter.🔗 Analysis chain
Ensure all usages of
ToAsciiTableare updated with the new parameterThe
ToAsciiTablemethod now includes an optionalmaxColumnWidthparameter. Please verify that all calls to this method throughout the codebase are reviewed to determine if they need to specify this parameter explicitly, especially if they rely on a different column width.Run the following script to find all usages of
ToAsciiTableand check if themaxColumnWidthparameter is correctly handled:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of ToAsciiTable and review if maxColumnWidth parameter is properly utilized. # Search for calls to the ToAsciiTable extension method rg --type csharp -A 2 -B 2 '\.ToAsciiTable\('Length of output: 4160
⛔ Skipped due to learnings
Learnt from: elsand PR: digdir/dialogporten#1232 File: src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs:0-0 Timestamp: 2024-10-07T07:45:33.970Z Learning: The `ToAsciiTable` method creates a copy of the input `rows` and operates on it, so the original `rows` passed to the method remains unchanged.Learnt from: elsand PR: digdir/dialogporten#1232 File: src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs:12-12 Timestamp: 2024-10-07T07:31:46.700Z Learning: In C# 12, square brackets `[]` can be used for array initialization, e.g., `string[] array = [ "value" ];`.
...ir.Tool.Dialogporten.SlackNotifier.Tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests.csproj
Show resolved
Hide resolved
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs
Show resolved
Hide resolved
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs
Show resolved
Hide resolved
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs
Outdated
Show resolved
Hide resolved
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs
Outdated
Show resolved
Hide resolved
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs (7)
7-34: LGTM: Well-structured test for text wrappingThis test effectively verifies that text exceeding the maximum column width is wrapped correctly. The expected output accurately represents the desired behavior.
Consider adding an additional assertion to explicitly check the width of each line:
Assert.True(result.Split('\n').All(line => line.Length <= maxColumnWidth + 2), "All lines should be within the maximum width (including border characters)");This would provide an extra layer of validation for the wrapping behavior.
36-61: Consider adding a test for multiple long wordsThis test effectively verifies that a single long word is not broken when it exceeds the maximum column width. To further strengthen this test suite, consider adding a case with multiple long words in a single cell. This would ensure that the wrapping behavior is correct when dealing with a mix of long and short words.
Example:
var rows = new List<List<object>> { new() { "Header1" }, new() { "Supercalifragilisticexpialidocious is a long word" } };This addition would help verify that the method correctly handles complex scenarios with mixed word lengths.
63-88: LGTM: Good test for null handlingThis test effectively verifies that null values in cells are handled gracefully. The expected output correctly shows an empty cell for the null value.
To make this test even more robust, consider adding an assertion to explicitly check for the absence of the string "null" in the output:
Assert.DoesNotContain("null", result, StringComparison.OrdinalIgnoreCase);This additional check would ensure that null values are truly represented as empty cells and not as the string "null".
90-115: Consider adding a test with mixed empty and non-empty cellsThis test effectively verifies the handling of a single empty string in a cell. To further enhance the test suite, consider adding a case with a mix of empty and non-empty cells in a single row. This would ensure that the table formatting remains correct when dealing with varied cell contents.
Example:
var rows = new List<List<object>> { new() { "Header1", "Header2", "Header3" }, new() { "", "Non-empty", "" } };This addition would help verify that the method correctly handles more complex scenarios with mixed cell contents.
117-142: LGTM: Good test for numeric right alignmentThis test effectively verifies that numeric values are right-aligned within cells. The expected output correctly demonstrates the desired alignment.
To make this test more comprehensive, consider adding a case with a mix of numeric and non-numeric values:
var rows = new List<List<object>> { new() { "Header1", "Header2" }, new() { 12345, "Text" } };This would ensure that the right alignment is applied only to numeric values while other types of content remain left-aligned.
145-172: Enhance test for non-string object handlingThis test effectively verifies that non-string objects are converted to strings and wrapped correctly. To further improve this test:
- Add an explicit check for the conversion, as suggested in the previous review:
Assert.Contains(1234567890.ToString(), result, StringComparison.Ordinal);
- Consider adding a test case with a custom object that overrides
ToString():public class CustomObject { public override string ToString() => "Custom string representation"; } // In the test method var rows = new List<List<object>> { new() { "Header1" }, new() { new CustomObject() } };This would ensure that the
ToAsciiTablemethod correctly handles various types of non-string objects.
1-172: Overall, excellent test coverage with room for minor enhancementsThe
AsciiTableFormatterTestsclass provides a comprehensive suite of tests for theToAsciiTablemethod. Each test is well-structured and focuses on a specific functionality or edge case. The suggestions provided in the individual test reviews will further enhance the robustness of this test suite.To complete this test suite, consider adding the following:
- A test for tables with multiple columns of varying widths.
- A test with Unicode characters to ensure proper handling of non-ASCII text.
- A test with a very large dataset to verify performance and memory usage.
These additions would provide even more thorough coverage of the
ToAsciiTablemethod's capabilities and edge cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs (1 hunks)
🧰 Additional context used
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs (4)
36-61: LGTM: Effective test for handling long wordsThis test successfully verifies that long words are not broken when they exceed the maximum column width. It's an important edge case to cover.
As suggested in a previous review, consider adding an assertion to check the actual width of the resulting table:
Assert.True(result.Split('\n')[1].Length > maxColumnWidth, "Table width should exceed maxColumnWidth for long words");This additional check would further validate the behavior and ensure that the table expands to accommodate long words.
90-115: LGTM: Good test for empty string handlingThis test effectively verifies that the
ToAsciiTablemethod handles empty strings correctly by representing them as empty cells.As suggested in a previous review, consider adding an explicit assertion to check that the cell maintains its structure:
Assert.Contains("| |", result, StringComparison.Ordinal);This additional check would ensure that the empty cell is correctly formatted within the table structure.
145-171: LGTM: Good test for non-string object handling, but could be more explicitThis test effectively verifies that non-string objects are converted to strings and wrapped correctly within the table. It's an important test for ensuring the
ToAsciiTablemethod can handle various data types.As suggested in a previous review, consider adding an explicit check for the conversion:
var nonStringObject = 1234567890; Assert.Contains(nonStringObject.ToString(), result, StringComparison.Ordinal);This additional assertion would explicitly verify that the non-string object was converted to a string during the table formation.
1-202: Overall, excellent test coverage with room for minor enhancementsThe
AsciiTableFormatterTestsclass provides a comprehensive suite of tests for theToAsciiTablemethod. The tests cover various aspects including text wrapping, handling of long words, null values, empty strings, numeric right alignment, non-string object conversion, and jagged table handling. Each test is well-structured and focuses on a specific functionality or edge case.To further enhance the test suite, consider adding the following tests as suggested in a previous review:
- A test for multiple columns to ensure proper alignment and separation.
- A test with a very large dataset to verify performance and memory usage.
- A test for special characters (e.g., Unicode) to ensure proper handling and alignment.
- A test for extremely small
maxColumnWidthvalues (e.g., 1 or 2) to verify graceful handling of edge cases.These additional tests would provide even more robust coverage of the
ToAsciiTablemethod's capabilities and edge cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs (2 hunks)
- tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs (1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs (2)
Learnt from: elsand PR: digdir/dialogporten#1232 File: src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs:0-0 Timestamp: 2024-10-07T07:45:33.970Z Learning: The `ToAsciiTable` method creates a copy of the input `rows` and operates on it, so the original `rows` passed to the method remains unchanged.Learnt from: elsand PR: digdir/dialogporten#1232 File: src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs:12-12 Timestamp: 2024-10-07T07:31:46.700Z Learning: In C# 12, square brackets `[]` can be used for array initialization, e.g., `string[] array = [ "value" ];`.
🔇 Additional comments (3)
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs (3)
7-34: LGTM: Well-structured test for text wrappingThis test effectively verifies that the
ToAsciiTablemethod correctly wraps text when it exceeds the specified maximum column width. The test case is well-structured with clear Arrange, Act, and Assert sections, and the expected output accurately represents the desired behavior.
63-88: LGTM: Proper null value handling testThis test effectively verifies that the
ToAsciiTablemethod handles null values gracefully by representing them as empty cells. The expected output correctly demonstrates this behavior, ensuring that the method doesn't throw exceptions when encountering null values.
173-202: LGTM: Excellent test for jagged table handlingThis test effectively verifies that the
ToAsciiTablemethod correctly handles jagged tables (rows with different numbers of columns) by padding them with empty cells. The expected output accurately represents the desired behavior, ensuring that the table structure is maintained even with inconsistent row lengths.
tests/Digdir.Tool.Dialogporten.SlackNotifier.Tests/AsciiTableFormatterTests.cs
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Show resolved
Hide resolved
src/Digdir.Tool.Dialogporten.SlackNotifier/Common/AsciiTableFormatter.cs
Show resolved
Hide resolved
|
Description
This introduces a soft-wrap parameter specifying the desired number of columns, and handling of linebreaks within the values in the tables. In short, instead of this:
we get this:
This will make copy/pasting from Slack harder, but will increase readability. It is assumed that copy/pasting can be done from Application Insights.
Note that it will not break long words (strings) without spaces, regardless of column max width, so for the above examples the effective max col width is 109 (ie. the length of
Infrastructure.Altinn.Authorization.AltinnAuthorizationClient+<PerformAuthorizedPartiesRequest>d__13.MoveNext)Much of this code is courtesy of ChatGPT.
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
Tests
AsciiTableFormatterclass, covering various scenarios for theToAsciiTablemethod.