Fallback to textdiffer when csv parsing fails #5876
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (1)
app/helpers/renderers/feedback_table_renderer.rb (1)
325-328: Consider logging the CSV parsing error when falling back toTextDifferIn both the
render_acceptedanddiffermethods, when aCSV::MalformedCSVErroris rescued, the exception is silently ignored. Logging the error would aid in debugging and monitoring potential issues with CSV parsing in the future.Apply this diff to add logging:
rescue CSV::MalformedCSVError # default to text if parsing failed + Rails.logger.warn("CSV parsing failed in render_accepted for expected output.") endrescue CSV::MalformedCSVError # default to text if parsing failed + Rails.logger.warn("CSV parsing failed in differ for generated or expected output.") endAlso applies to: 336-339
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/helpers/renderers/feedback_table_renderer.rb (2 hunks)
- test/controllers/submissions_controller_test.rb (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/helpers/renderers/feedback_table_renderer.rb (2)
346-346: Refactoredtest_acceptedmethod enhances code readability and modularityBy extracting the rendering logic for accepted tests into the
render_acceptedmethod, the code becomes more modular and easier to maintain. This improves readability and aligns with best practices.
334-341: 🛠️ Refactor suggestionAvoid naming the variable
differto prevent confusion with the method nameIn the
diffmethod, the local variabledifferis assigned the result of thediffer(t)method. This naming can be confusing and may lead to misunderstandings or errors. Consider renaming the variable to something likedifferenceordiffer_instanceto improve code clarity.Apply this diff to rename the variable:
def diff(t) - differ = differ(t) + difference = differ(t) @builder.div(class: "diffs show-#{@diff_type}") do - @builder << differ.split - @builder << differ.unified + @builder << difference.split + @builder << difference.unified end endLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/helpers/renderers/feedback_table_renderer.rb (1)
330-331: Consider logging CSV parsing errors for improved diagnosticsWhen a
CSV::MalformedCSVErroris rescued, logging the error can aid in diagnosing issues related to malformed CSV inputs. This will help in monitoring and troubleshooting potential problems with CSV data.Apply this diff to log the exception:
-rescue CSV::MalformedCSVError +rescue CSV::MalformedCSVError => e + Rails.logger.warn("CSV parsing failed: #{e.message}") yield(TextDiffer) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/helpers/renderers/feedback_table_renderer.rb (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/helpers/renderers/feedback_table_renderer.rb
[warning] 328-328: app/helpers/renderers/feedback_table_renderer.rb#L328
Added line #L328 was not covered by tests
🔇 Additional comments (1)
app/helpers/renderers/feedback_table_renderer.rb (1)
322-334: Refactored error handling enhances CSV parsing robustnessThe introduction of the
with_differ_classmethod and its integration intotest_acceptedanddiffmethods significantly improves error handling. By gracefully handling malformed CSV data and appropriately falling back toTextDiffer, the code becomes more robust and maintainable.Also applies to: 340-342, 353-358
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 328-328: app/helpers/renderers/feedback_table_renderer.rb#L328
Added line #L328 was not covered by tests
| if CsvDiffer.limited_columns?(t[:generated]) && CsvDiffer.limited_columns?(t[:expected]) | ||
| yield(CsvDiffer) | ||
| else | ||
| yield(TextDiffer) |
There was a problem hiding this comment.
Add tests to cover fallback when CSV column limits are exceeded
The else branch in the with_differ_class method, where yield(TextDiffer) is called due to column limits being exceeded, is not currently covered by tests as indicated by the static analysis tool. Adding tests for this scenario will ensure that the fallback to TextDiffer functions correctly when CSV inputs exceed column limitations.
Would you like assistance in creating test cases to cover this condition?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 328-328: app/helpers/renderers/feedback_table_renderer.rb#L328
Added line #L328 was not covered by tests
|
I removed the code duplication and also made the choice for the TextDiffer explicit I think the deduplication is an improvement but I personally prefer the the implicit fallback as it simplifies the amount of cases, because most cases have the same result. With implicit fallback the method would look like this: def with_differ_class(t)
if t[:format] == 'csv'
begin
return yield(CsvDiffer) if CsvDiffer.limited_columns?(t[:generated]) && CsvDiffer.limited_columns?(t[:expected])
rescue CSV::MalformedCSVError
# use the default differ
end
end
yield(TextDiffer)
end |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/helpers/renderers/feedback_table_renderer.rb (2)
322-331: LGTM! Consider adding a comment for clarity.The implementation of
with_differ_classlooks good. It correctly handles the CSV format case and implements the fallback to TextDiffer as requested. The error handling for malformed CSV is also appropriate.Consider adding a brief comment explaining the fallback logic, e.g.:
# Attempt to use CsvDiffer for 'csv' format, fallback to TextDiffer if parsing fails or for other formats
349-354: LGTM! Consider a minor optimization.The changes to the
diffmethod look good. It correctly uses the newwith_differ_classmethod and renders both split and unified diffs.Consider creating the differ instance outside the div to potentially improve performance:
with_differ_class(t) do |differ_class| differ = differ_class.new(t[:generated], t[:expected]) @builder.div(class: "diffs show-#{@diff_type}") do @builder << differ.split @builder << differ.unified end endThis way, the differ instance is created only once, regardless of how many times its methods are called.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/helpers/renderers/feedback_table_renderer.rb (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
app/helpers/renderers/feedback_table_renderer.rb (3)
335-339: LGTM! Good use of the newwith_differ_classmethod.The changes to
test_acceptedlook good. The method now correctly uses the newwith_differ_classmethod, which allows for the appropriate differ class to be used based on the test format.
322-331: Reminder: Add tests for CSV fallback scenarioWhile the implementation of the fallback logic looks good, we should ensure it's covered by tests, especially for the case when CSV column limits are exceeded.
Would you like assistance in creating test cases to cover this condition? We could add tests to ensure that:
- CsvDiffer is used when the format is 'csv' and column limits are met.
- TextDiffer is used when the format is 'csv' but column limits are exceeded.
- TextDiffer is used when a CSV::MalformedCSVError is raised.
Let me know if you'd like me to propose some test cases or open a GitHub issue to track this task.
Line range hint
322-354: Overall, good implementation with room for minor improvementsThe changes effectively implement the fallback mechanism for CSV parsing as requested. The code is generally well-structured and readable. Consider the following suggestions for improvement:
- Add a brief comment in
with_differ_classto explain the fallback logic.- Optimize the
diffmethod by creating the differ instance outside the div.- Add tests to cover the CSV fallback scenarios, including when column limits are exceeded and when CSV parsing fails.
These minor enhancements will improve code clarity, potentially boost performance, and ensure robustness through proper test coverage.
This pull request lets the differ fallback to the Textdiffer in case the csv is malformed.
This issue was detected by submissions from the postgres judge, which returned to many escaping characters before a newline for a while. This issue is already resolved in the judge as well.
Example of a problematic submission: https://dodona.be/en/submissions/19072577.json