fix(cli): validate --exclude regex pattern before use#251
fix(cli): validate --exclude regex pattern before use#251rafaeltonholo merged 2 commits intorafaeltonholo:mainfrom
Conversation
Invalid regex patterns passed to --exclude caused an unhandled
PatternSyntaxException with a stack trace. Added Clikt .validate {}
to catch the error early and display a clear message:
Error: Invalid regex pattern: "[invalid". Unclosed character class...
Fixes rafaeltonholo#239
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded validation for the CLI Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
svg-to-compose/src/nativeMain/kotlin/Main.kt (1)
148-157: Consider using.convert {}to avoid compiling the regex twice.The validation block compiles the regex to check validity, then discards it. Later at line 246, the same pattern is compiled again via
exclude?.let(::Regex). Using.convert {}would validate and return theRegexin a single pass.That said, the current implementation is correct and achieves the PR objective of providing clear error messages for invalid patterns.
♻️ Optional: Single-pass validation and conversion
private val exclude by option( names = arrayOf("--exclude"), help = "A regex used to exclude some icons from the parsing.", -).validate { pattern -> +).convert { pattern -> try { Regex(pattern) } catch (e: Exception) { fail("Invalid regex pattern: \"$pattern\". ${e.message}") } }Then at line 246, simplify to:
- exclude = exclude?.let(::Regex), + exclude = exclude,Also applies to: 246-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@svg-to-compose/src/nativeMain/kotlin/Main.kt` around lines 148 - 157, The exclude option currently uses .validate { ... } which compiles the pattern only to check validity and then later recompiles it with exclude?.let(::Regex); change the option to use .convert { pattern -> try { Regex(pattern) } catch (e: Exception) { fail("Invalid regex pattern: \"$pattern\". ${e.message}") } } so the option returns a Regex (named exclude) directly, and then remove the later exclude?.let(::Regex) call and use exclude (or exclude?) as a Regex value wherever referenced (e.g., the code that previously used exclude?.let(::Regex) at the later usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@svg-to-compose/src/nativeMain/kotlin/Main.kt`:
- Around line 148-157: The exclude option currently uses .validate { ... } which
compiles the pattern only to check validity and then later recompiles it with
exclude?.let(::Regex); change the option to use .convert { pattern -> try {
Regex(pattern) } catch (e: Exception) { fail("Invalid regex pattern:
\"$pattern\". ${e.message}") } } so the option returns a Regex (named exclude)
directly, and then remove the later exclude?.let(::Regex) call and use exclude
(or exclude?) as a Regex value wherever referenced (e.g., the code that
previously used exclude?.let(::Regex) at the later usage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b7ae189-40f1-43e0-b789-efa25474f36f
📒 Files selected for processing (1)
svg-to-compose/src/nativeMain/kotlin/Main.kt
| } catch (e: Exception) { | ||
| fail("Invalid regex pattern: \"$pattern\". ${e.message}") | ||
| } |
There was a problem hiding this comment.
Hi @mvanhorn, we should avoid using a too Generic Exception handler because it can cause problems.
Here we should be catching the PatternSyntaxException instead.
There was a problem hiding this comment.
Narrowed to IllegalArgumentException in 32f89fb. On Kotlin/Native, Regex() throws IllegalArgumentException for invalid patterns (there's no PatternSyntaxException on native targets - that's JVM-only, where it extends IllegalArgumentException). So this is the tightest catch available here.
Narrow the catch clause for regex validation from generic Exception to IllegalArgumentException, which is what Kotlin/Native's Regex constructor throws for invalid patterns. On JVM, PatternSyntaxException extends IllegalArgumentException, so this is the correct narrowing for native targets.
rafaeltonholo
left a comment
There was a problem hiding this comment.
LGTM! Thanks for addressing the comment!
|
Thanks for the review and merge! Glad the regex validation approach worked out. |
Summary
Added Clikt
.validate {}to the--excludeoption so invalid regex patterns produce a clear error instead of an unhandled exception.Why this matters
Running
./s2c --exclude "[invalid" ...threw aPatternSyntaxExceptionwith a stack trace. Now it shows:Error: Invalid regex pattern: "[invalid". Unclosed character class near index 8.Changes
svg-to-compose/src/nativeMain/kotlin/Main.kt: Added.validate { }block to theexcludeoption (line 150) that tries constructing aRegexand callsfail()on error. Addedvalidateimport.Testing
The validation runs at option-parsing time before any file processing begins. Invalid patterns now produce Clikt's standard error format.
Fixes #239
This contribution was developed with AI assistance (Claude Code).
Summary by CodeRabbit