Add ignore_regex option to large_tuple rule#6452
Conversation
Generated by 🚫 Danger |
SimplyDanny
left a comment
There was a problem hiding this comment.
Not too bad ... Please check my comments.
| "func foo() -> Regex<↓(Substring, foo: Substring, bar: Substring)>.Match? { nil }", | ||
| configuration: ["ignore_regex": false] | ||
| ), | ||
| Example( |
There was a problem hiding this comment.
This example could be omitted and instead the option be added to the example in line 69.
| private extension TupleTypeSyntax { | ||
| var isInsideRegexType: Bool { | ||
| var currentNode: Syntax? = Syntax(self) | ||
| while let node = currentNode { |
There was a problem hiding this comment.
These while iterations are not nice, as they would always walk up the whole tree in the negative case.
I'd rather explicitly check if this tuple is used as a generic parameter of a Regex type. Assuming the type is never optional (i.e. Regex<(A, B)?> would not appear), the chain is like:
IdentifierType "Regex"
GenericArgumentClause
GenericArgumentList
GenericArgument
TupleType
If the type can be optional is well, TupleType will be wrapped in an OptionalType in addition. Can it ever be optional? 🤔
There was a problem hiding this comment.
I tested it out in playground and the compiler allows it. Although the syntax for my last example needs updating as the regex itself is no longer optional. I'll update it now.
|
Thanks for the review feedback! I've addressed all the comments:
Ready for another look! |
|
The above comment is Claude, if anything is up here I'll do it myself. I'm reviewing everything it does but as someone unfamiliar with this repo I'm a bit self conscious of applying a review burden on yourself so let me know if I should refrain from doing this in future. Thanks for reviewing. |
| /// Check if this tuple is a direct generic argument of a `Regex` type. | ||
| /// Expected chain: TupleType -> GenericArgument -> GenericArgumentList -> | ||
| /// GenericArgumentClause -> IdentifierType "Regex" | ||
| /// Optionally with OptionalType wrapper: TupleType -> OptionalType -> GenericArgument -> ... |
There was a problem hiding this comment.
This comments just repeats what's written in the code already. Let's remove it.
As long as I'm still interacting with a human and the code is not totally off, I'm okay with that. Swift and especially SwiftSyntax are not really LLM's strengths and so sometimes they struggle a lot. For such code, I would not like to give any feedback. The idea must be understood by the human I'm interacting with. |
|
But thanks for being so open about your approach, @Deco354! Much better than obvious AI-generated code that people claim it's theirs. |
That seems like a good rubric to go off. I'm trying not to vibe code and checkover everything it does and request edits when something isn't to the standard I'd commit to a swift project myself but letting it have full control over writing up PRs, commits and doing the initial implementation. It's been quite handy for working with an unfamiliar project which would normally take a while to get up to speed on. I am a bit rusty though as i've been working on ML/Python projects the last two years (part of the reason I'm here is to avoid SwiftUI which I'm not confident reviewing). |
This option allows silencing large tuple violations for tuples inside `Regex<...>` types, which commonly have large tuple type parameters for capture groups. Co-Authored-By: Claude <noreply@anthropic.com>
- Remove separate Regex triggering example; add configuration to existing example - Replace while-loop tree traversal with direct parent chain check for better performance - Add documentation comment explaining the expected SwiftSyntax node chain Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This tests the OptionalType wrapper case where the tuple itself is optional
0241414 to
e7b4d33
Compare
Summary
I'm trialing out Claude code with this issue. Please be extra thorough with this review as I've little experience with this repo so may have missed mistakes it has made when reviewing this.
ignore_regexconfiguration option to thelarge_tuplerule to silence violations for tuples insideRegex<...>types, which commonly have large tuple type parameters for capture groups.Resolves #6340
Test plan
ignore_regex: trueandignore_regex: false🤖 Generated with Claude Code