Fix try/rescue error handling anti-patterns - #3
Merged
Merged
Conversation
- Convert parse_integer_exact!/1 and parse_float_exact!/1 to warning-based fallbacks instead of raising exceptions - Add proper error handling with type conversion functions - Add logging for parsing failures instead of crashing - Improve convert_value functions with safe parsing using Integer.parse/1 and Float.parse/1 - Add proper typespec annotations for better dialyzer support This eliminates exception-throwing anti-patterns in core parsing utilities while maintaining backward compatibility.
- Replace Map.fetch! with Map.get and proper fallbacks in codec mapping functions - Replace File.mkdir_p! with File.mkdir_p and error handling with fallback to system temp - Add proper error logging and graceful degradation - Update typespec annotations to reflect new return types - Remove dangerous exception-throwing patterns from core utility functions These changes prevent crashes when codecs are unknown or temp directories cannot be created.
- Remove unnecessary rescue blocks around JSON decoding in Broadway analyzers (Jason.decode already returns error tuples) - Replace try/catch blocks with proper pattern matching and error handling - Remove redundant rescue blocks from dashboard state and presenter modules - Fix compilation warnings by removing unreachable error patterns - Simplify error handling in LiveView components by removing defensive rescue blocks - Replace catch blocks with proper GenServer.call error handling - Update service modules to use safe pattern matching instead of exception-based control flow - Fix credo issues: eliminate negated conditions, reduce function complexity, remove explicit try blocks - Refactor complex functions into smaller, more maintainable helper functions This eliminates try/rescue anti-patterns while preserving legitimate error boundaries for system health checks, resource cleanup, and external API resilience.
There was a problem hiding this comment.
Pull Request Overview
This PR systematically eliminates try/rescue error handling anti-patterns throughout the Reencodarr codebase by removing unnecessary rescue blocks and replacing dangerous bang functions with safe alternatives.
Key changes:
- Removes unnecessary try/rescue blocks around functions that already return error tuples
- Replaces
Map.fetch!,String.to_atom, andFile.mkdir_p!with safer alternatives that handle errors gracefully - Refactors complex conditional chains into smaller helper functions for better maintainability
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
test/support/test_helpers.ex |
Removes unnecessary try/catch and moves File.write! outside try block |
test/reencodarr/video_processing_pipeline_test.exs |
Removes redundant try blocks around mocked functions |
test/reencodarr/sync_integration_test.exs |
Removes unnecessary try/rescue blocks for service refresh operations |
test/reencodarr/analyzer/broadway/error_handling_test.exs |
Removes try/rescue blocks around Broadway operations |
test/reencodarr/ab_av1/crf_search_retry_test.exs |
Replaces try/catch with pattern matching for mock cleanup |
lib/reencodarr_web/live/failures_live.ex |
Refactors complex query function into smaller helper functions |
lib/reencodarr_web/live/dashboard_live.ex |
Replaces generic safe_call pattern with specific rescue blocks |
lib/reencodarr_web/live/components/manual_scan_component.ex |
Removes try/rescue around ManualScanner.scan that already returns proper errors |
lib/reencodarr_web/dashboard/presenter.ex |
Replaces rescue with safe ETS table existence check |
lib/reencodarr/services/sonarr.ex |
Replaces bang functions with safe parsing and proper error handling |
lib/reencodarr/services/radarr.ex |
Replaces bang functions with safe parsing and proper error handling |
lib/reencodarr/media/video_validator.ex |
Uses String.to_existing_atom instead of String.to_atom with rescue |
lib/reencodarr/media/video_upsert.ex |
Uses String.to_existing_atom with proper error handling for unknown keys |
lib/reencodarr/media/codecs.ex |
Replaces Map.fetch! with Map.get and default value |
lib/reencodarr/media/codec_mapper.ex |
Replaces Map.fetch! with Map.get and default value |
lib/reencodarr/media.ex |
Replaces rescue with catch for comprehensive error handling |
lib/reencodarr/encoder/broadway/producer.ex |
Removes try/catch around GenServer call and simplifies pattern matching |
lib/reencodarr/dashboard_state.ex |
Adds case guards to ensure boolean return values from Broadway status checks |
lib/reencodarr/core/parsers.ex |
Replaces dangerous parsing bang functions with safe alternatives |
lib/reencodarr/analyzer/broadway.ex |
Removes extensive try/rescue blocks and simplifies error handling |
lib/reencodarr/ab_av1/helper.ex |
Replaces File.mkdir_p! with safe version and fallback logic |
lib/reencodarr/ab_av1/crf_search.ex |
Removes try/catch around GenServer call and adds proper timeout |
- Replaced all usages with safe parse_float_exact/1 and proper error handling - Updated parse_crf functions in media.ex and clean.ex to handle parsing errors gracefully - Eliminated the problematic bang function that was causing review concerns - All tests pass, no credo issues
- Return {:ok, parsed_ids} tuple on success to match typespec
- Addresses reviewer feedback about inconsistent return type
- All tests pass, no credo issues
- Completely removed parse_integer_exact and parse_float_exact bang functions - Updated all usages to use safe parsing with proper error handling - Fixed try/rescue patterns in video_validator.ex and video_upsert.ex with safer alternatives - Added proper @deprecated attribute for any remaining deprecated functions - All tests pass, no compilation warnings
- Remove try/rescue from produces_invalid_audio_args? (let it crash approach) - Update filter functions in failures_live.ex to return error tuples instead of :unknown - Fix parse_crf to use pattern matching for let-it-crash behavior
mjc
added a commit
that referenced
this pull request
Mar 5, 2026
Eliminate duplicated format function implementations by using delegation: File Size Formatting Consolidation: - Replace identical format_file_size() implementations across 3 modules - Consolidate format_savings_bytes() logic duplicated in 2 modules - Standardize on ReencodarrWeb.FormatHelpers as canonical implementation - Remove ~50 lines of duplicated byte formatting logic Count/Metric Formatting Consolidation: - Consolidate format_count() K/M suffix logic across modules - Unify format_fps(), format_eta(), format_score() implementations - Use defdelegate pattern to eliminate function duplication - Maintain API compatibility while removing code duplication This addresses #2 and #3 highest-impact duplication patterns: - File Size Formatting Duplication (8+ files affected) - Count/Metric Formatting Duplication (6+ files affected) Benefits: Single source of truth for formatting, easier maintenance, consistent behavior across the application.
mjc
added a commit
that referenced
this pull request
Mar 5, 2026
Resolves critical duplication pattern #3 from DUPLICATION_ANALYSIS.md VALIDATION CONSOLIDATION SUMMARY: - Removed unused lib/reencodarr/changeset_helpers.ex (195 lines eliminated) - Cleaned duplicate validation functions from lib/reencodarr/utils.ex (30+ lines) - Fixed corrupted docstring in lib/reencodarr/validation.ex - Established Reencodarr.Validation as single source of truth DUPLICATE FUNCTIONS ELIMINATED: - validate_positive_number/3, validate_required_field/3, validate_not_empty/3 - validate_audio_channels/1, validate_video_resolution/1, validate_track_consistency/2 - Plus 8+ other validation utilities across 3 modules ARCHITECTURE IMPROVEMENTS: - Single, authoritative validation module (actively used in 5 MediaInfo modules) - Domain-specific validations appropriately kept inline where contextually relevant - Eliminated 225+ lines of duplicate validation code VALIDATION: - All 369 tests passing, no functionality lost - No breaking changes to existing validation usage - Maintained backward compatibility for all MediaInfo modules Impact: 225+ lines of duplicate validation code eliminated, improved maintainability
mjc
added a commit
that referenced
this pull request
Mar 5, 2026
Fix try/rescue error handling anti-patterns
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Systematically eliminates try/rescue error handling anti-patterns throughout the Reencodarr codebase while maintaining appropriate error boundaries.
Key improvements:
All tests pass, no credo issues, maintains backward compatibility.