feat: regular expr on exercise #115
Conversation
WalkthroughAdds a new "Regular Expressions" exercise: catalog entries, template (stub) and tests, plus a reference solution in Go exposing MatchRegex(pattern, input string) bool using the regexp package. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as Test Runner
participant Impl as MatchRegex
participant Std as regexp (stdlib)
Tester->>Impl: MatchRegex(pattern, input)
Impl->>Std: Compile(pattern)
alt compile error
Std-->>Impl: error
Impl-->>Tester: false
else compiled
Std-->>Impl: *Regexp
Impl->>Std: MatchString(input)
Std-->>Impl: bool
Impl-->>Tester: result
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
![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
🧹 Nitpick comments (3)
internal/exercises/catalog.yaml (1)
205-213: Consider adding difficulty and topics metadata.Unlike the
109_epochexercise above (lines 196-203), this entry lacksdifficultyandtopicsfields. Adding them would improve consistency and help learners filter exercises.Example addition:
- slug: regular-expressions title: Regular Expressions + difficulty: beginner + topics: ["regexp", "pattern matching"] test_regex: ".*" hints:internal/exercises/templates/regular-expressions/regular-expressions_test.go (1)
5-26: LGTM! Consider adding tests for invalid patterns.The test structure is idiomatic and covers typical regex use cases. The test cases are appropriate for a beginner exercise.
However, the current implementation stub in
internal/exercises/templates/regular-expressions/regular-expressions.go(line 10) does not return an error. If learners are expected to handle invalid patterns (e.g., unclosed brackets like"[0-9"), consider either:
- Adding test cases for invalid patterns and expecting a specific behavior (e.g., returning false or panicking), or
- Updating the function signature to return
(bool, error)and adding error-handling tests.Example test case if sticking with
boolreturn:{"[0-9", "123", false}, // invalid pattern, expect false or panicinternal/exercises/templates/regular-expressions/regular-expressions.go (1)
9-13: Enhance TODO comment to guide error handling.The function signature returns only
bool, so learners usingregexp.MustCompileon invalid patterns will face panics. Enhance the TODO to suggest usingregexp.Compileand checking errors, or clarify thatMustCompileis acceptable for trusted patterns.Apply this diff:
// MatchRegex returns true if the input matches the pattern, false otherwise. func MatchRegex(pattern, input string) bool { - // TODO: implement this function + // TODO: implement this function using regexp.Compile and check for errors, + // or use regexp.MustCompile if you are certain the pattern is valid. return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exercises/catalog.yaml(1 hunks)internal/exercises/templates/regular-expressions/regular-expressions.go(1 hunks)internal/exercises/templates/regular-expressions/regular-expressions_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/exercises/templates/regular-expressions/regular-expressions_test.go (1)
internal/exercises/templates/regular-expressions/regular-expressions.go (1)
MatchRegex(10-13)
zhravan
left a comment
zhravan
left a comment
There was a problem hiding this comment.
Overall LGTM, can you please add solutions to it as well?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exercises/solution/regular-expressions_solution.go (2)
9-12: Consider explicit error handling for invalid patterns.Returning
falsefor both compilation errors and non-matches conflates two distinct failure modes. Callers cannot distinguish "invalid pattern" from "valid pattern, no match." For an exercise solution, consider whether teaching explicit error propagation would be more valuable.If patterns are expected to always be valid (e.g., hardcoded in tests), use
regexp.MustCompileto panic on invalid patterns:func MatchRegex(pattern, input string) bool { - re, err := regexp.Compile(pattern) - if err != nil { - return false - } + re := regexp.MustCompile(pattern) return re.MatchString(input) }Alternatively, if invalid patterns should be handled gracefully, return an error to the caller:
-func MatchRegex(pattern, input string) bool { +func MatchRegex(pattern, input string) (bool, error) { re, err := regexp.Compile(pattern) if err != nil { - return false + return false, err } - return re.MatchString(input) + return re.MatchString(input), nil }
9-13: Performance: Pattern compilation on every call.
regexp.Compileis invoked on every call, which can be expensive for repeated matches with the same pattern. For production code, consider compiling once and reusing the regex. However, if this is intentional for the exercise (to teach basic regex usage), this is acceptable.If performance matters and patterns are reused, consider caching compiled regexes or accepting a pre-compiled
*regexp.Regexp:// Example: Accept pre-compiled regex func MatchRegex(re *regexp.Regexp, input string) bool { return re.MatchString(input) }Or use
regexp.MatchStringfor simple one-off matches:func MatchRegex(pattern, input string) bool { - re, err := regexp.Compile(pattern) - if err != nil { - return false - } - return re.MatchString(input) + matched, err := regexp.MatchString(pattern, input) + if err != nil { + return false + } + return matched }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exercises/catalog.yaml(1 hunks)internal/exercises/solution/regular-expressions_solution.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exercises/catalog.yaml
|
@JoanJaraBosch : Can you please follow the naming convention as per the files please? |
I think now it is correct |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exercises/templates/regular_expressions/regular_expressions_test.go (1)
5-26: LGTM! Consider adding edge case coverage.The test structure follows Go table-driven test conventions correctly. The current test cases cover anchors, character classes, and alternation patterns effectively.
For more comprehensive coverage, consider adding tests for:
- Empty pattern and empty input
- Invalid regex patterns (to verify error handling)
- Quantifiers:
*,+,?,{n,m}- Escaped special characters:
\.,\d,\w- Boundary matchers:
\bAdditionally, using subtests with
t.Run()would improve test isolation and provide clearer failure messages:- for _, tt := range tests { + for _, tt := range tests { + t.Run(tt.pattern+"_"+tt.input, func(t *testing.T) { got := MatchRegex(tt.pattern, tt.input) if got != tt.want { t.Errorf("MatchRegex(%q, %q) = %v; want %v", tt.pattern, tt.input, got, tt.want) } + }) }internal/exercises/solutions/regular_expressions/regular_expressions_solution.go (1)
7-14: LGTM! Implementation is correct.The solution correctly compiles the regex pattern and matches it against the input. Returning
falseon compilation errors provides graceful handling for invalid patterns, which aligns with the boolean return signature.For a more concise implementation, you could use
regexp.MatchStringdirectly, which handles compilation internally:-func MatchRegex(pattern, input string) bool { - re, err := regexp.Compile(pattern) - if err != nil { - return false - } - return re.MatchString(input) -} +func MatchRegex(pattern, input string) bool { + matched, _ := regexp.MatchString(pattern, input) + return matched +}However, the current approach is more explicit and educational for learners, showing the two-step process of compilation and matching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exercises/solutions/regular_expressions/regular_expressions_solution.go(1 hunks)internal/exercises/templates/regular_expressions/regular_expressions.go(1 hunks)internal/exercises/templates/regular_expressions/regular_expressions_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/exercises/templates/regular_expressions/regular_expressions.go (1)
internal/exercises/solutions/regular_expressions/regular_expressions_solution.go (1)
MatchRegex(8-14)
internal/exercises/solutions/regular_expressions/regular_expressions_solution.go (1)
internal/exercises/templates/regular_expressions/regular_expressions.go (1)
MatchRegex(10-13)
internal/exercises/templates/regular_expressions/regular_expressions_test.go (2)
internal/exercises/solutions/regular_expressions/regular_expressions_solution.go (1)
MatchRegex(8-14)internal/exercises/templates/regular_expressions/regular_expressions.go (1)
MatchRegex(10-13)
🔇 Additional comments (1)
internal/exercises/templates/regular_expressions/regular_expressions.go (1)
1-13: LGTM! Template structure is appropriate.The exercise template correctly provides:
- Clear file and package documentation
- The function signature matching the solution
- A TODO marker for learners to implement
- Necessary imports already in place
- A safe default return value
This scaffolding gives learners the right starting point without revealing the solution.
Note: The PR comment from @zhravan mentions following naming conventions. The current file uses
regular_expressions.go(snake_case), which is standard Go convention for filenames. If there's a specific project naming convention, please clarify.
zhravan
changed the title
zhravan
left a comment
zhravan
left a comment
There was a problem hiding this comment.
Rest lgtm, one minor change required, lmk once that is done.
Thank you for considering contributing to the project.
internal/exercises/solutions/regular_expressions/regular_expressions_solution.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exercises/templates/115_regular_expressions/regular_expressions_test.go (2)
7-28: LGTM! Consider using subtests for better test output.The test implementation is correct and covers basic regex functionality well. The table-driven approach follows Go best practices.
Consider refactoring to use subtests with
t.Run()for clearer output when tests fail:func TestMatchRegex(t *testing.T) { tests := []struct { + name string pattern string input string want bool }{ - {"^a.*z$", "abcz", true}, - {"^a.*z$", "abz", true}, - {"^a.*z$", "abc", false}, - {"[0-9]+", "123", true}, - {"[0-9]+", "abc", false}, - {"hello|world", "hello", true}, - {"hello|world", "world", true}, - {"hello|world", "hi", false}, + {"anchor with wildcard match", "^a.*z$", "abcz", true}, + {"anchor with wildcard shorter", "^a.*z$", "abz", true}, + {"anchor with wildcard no match", "^a.*z$", "abc", false}, + {"digit class match", "[0-9]+", "123", true}, + {"digit class no match", "[0-9]+", "abc", false}, + {"alternation first option", "hello|world", "hello", true}, + {"alternation second option", "hello|world", "world", true}, + {"alternation no match", "hello|world", "hi", false}, } for _, tt := range tests { - got := MatchRegex(tt.pattern, tt.input) - if got != tt.want { - t.Errorf("MatchRegex(%q, %q) = %v; want %v", tt.pattern, tt.input, got, tt.want) - } + t.Run(tt.name, func(t *testing.T) { + got := MatchRegex(tt.pattern, tt.input) + if got != tt.want { + t.Errorf("MatchRegex(%q, %q) = %v; want %v", tt.pattern, tt.input, got, tt.want) + } + }) } }
13-21: Consider adding edge case tests.The current test cases cover common regex patterns well. You might want to add tests for:
- Invalid regex patterns (e.g.,
"[", "(?P<", "*abc")- Empty strings (
"", "","a+", "")- Special characters that need escaping (
"\.", "hello.world")Example additions:
{"invalid pattern", "[", "test", false}, {"empty pattern and input", "", "", true}, {"empty input with pattern", "a+", "", false}, {"escaped special char", `\.`, ".", true},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exercises/solutions/115_regular_expressions/regular_expressions.go(1 hunks)internal/exercises/templates/115_regular_expressions/regular_expressions.go(1 hunks)internal/exercises/templates/115_regular_expressions/regular_expressions_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/exercises/templates/115_regular_expressions/regular_expressions.go (1)
internal/exercises/solutions/115_regular_expressions/regular_expressions.go (1)
MatchRegex(10-16)
internal/exercises/solutions/115_regular_expressions/regular_expressions.go (1)
internal/exercises/templates/115_regular_expressions/regular_expressions.go (1)
MatchRegex(12-15)
internal/exercises/templates/115_regular_expressions/regular_expressions_test.go (2)
internal/exercises/solutions/115_regular_expressions/regular_expressions.go (1)
MatchRegex(10-16)internal/exercises/templates/115_regular_expressions/regular_expressions.go (1)
MatchRegex(12-15)
🔇 Additional comments (2)
internal/exercises/solutions/115_regular_expressions/regular_expressions.go (1)
9-16: LGTM! Solution is correct and appropriate for educational context.The implementation correctly:
- Compiles the regex pattern using
regexp.Compile- Handles compilation errors by returning
false- Uses
MatchStringto test if the input matches the patternThis straightforward approach is well-suited for an educational exercise, making it easy for learners to understand the basic regex workflow.
internal/exercises/templates/115_regular_expressions/regular_expressions.go (1)
11-15: LGTM! Template provides clear scaffold for learners.The function stub is well-structured:
- Clear function signature matching the solution
- TODO comment guides learners on what to implement
regexppackage already imported for convenience- Placeholder return value allows compilation
This is an appropriate template for an educational exercise.
|
done @zhravan |
2 participants
Summary
Describe the change and its motivation.
Checklist
make verifyorgolearn verify <slug>Screenshots / Output (if CLI UX)
Paste before/after where helpful.
Related issues
Fixes #
Summary by CodeRabbit
New Features
Tests