Support file-level ignore directive for specific rules #950
Conversation
TTOzzi
requested review from
ahoppen,
allevato,
bnbarham and
shawnhyam
as code owners
TTOzzi
force-pushed
the
ignore-specific-rules-in-file
branch
from
618e38e to
ab12d60
Compare
| @@ -651,4 +651,36 @@ final class OrderedImportsTests: LintOrFormatRuleTestCase { | |||
| ] | |||
| ) | |||
| } | |||
|
|
|||
| func testImportsOrderWithFileIgnoreDirective() { | |||
| assertFormatting( | |||
There was a problem hiding this comment.
Does this also end up adding a newline after // swift-format-ignore-file if no re-ordering is necessary? Eg. for
// swift-format-ignore-file
import LibA
import LibBTo be clear, I’m fine with that, just want to check and record the behavior in a test case.
One thing I did notice is that I think this flips comments order around if you have eg.
// We need to ignore this file because it is generated
// swift-format-ignore-line
import FoundationMaybe swift-format-ignore-line just needs to flush the commentBuffer into the headerBuffer similar to what a blank line does.
| private var pattern: String { | ||
| return #"^\s*\/\/\s*"# + description + #"((:\s+(([A-z0-9]+[,\s]*)+))?$|\s+$)"# | ||
| } | ||
|
|
||
| /// Rule ignore regex object. | ||
| fileprivate var regex: NSRegularExpression { | ||
| return try! NSRegularExpression(pattern: pattern, options: []) | ||
| } |
There was a problem hiding this comment.
Since you are working on this already, I’ve got a few thoughts:
- The regex restriction after the
:seems a little restrictive to me. Eg. I imagine it would be fine to try and debug why a future rule that contains a_cannot be ignored using a directive. Maybe we can just make the regex a prefix match. @allevato Do you remember why the regex is this specific? - This re-compiles the regex every time
regexis accessed. Compiling regular expressions is fairly expensive, so I’d prefer to cache that. - Could we use Swift’s Regex instead of
NSRegularExpression? - Or, alternatively, would it be easier to write a simple line matcher and avoid going through regex matching at all?
All except for avoiding to re-compile the regex don’t need to be in this PR.
There was a problem hiding this comment.
@allevato Do you remember why the regex is this specific?
Not off the top of my head. It may have just been to capture precisely what we expected rule names to look like at the time. I think it would be fine to be a little less prescriptive in the regex itself, if it simplifies things.
Could we use Swift’s Regex instead of NSRegularExpression?
We'd have to raise our minimum deployment version, which is macOS 12.0 right now, since Regex requires 13.0. If that aligns with other binaries in the Swift toolchain, I'd love to drop NSRegularExpression (holistically, in a separate PR).
Resolve #927
I have modified the function that disables specific rules in node-level directive so that it can also be reused for file-level directive, ensuring both follow the same logic.
Previously, if a file-level directive appeared in the topmost comment, we would always skip visiting nodes. However, with this change, we now visit child nodes when only specific rules are disabled. As a result, this caused an unintended side effect in cases where the
OrderedImportsrule was enabled. Specifically, a directive included in theleadingTriviaof a sorted import statement was being reordered along with the imports.To resolve this, I modified the
OrderedImportsrule to treat theswift-format-ignore-filedirective as part of the file header.Additionally, to avoid simple string comparisons like:
I have created an
IgnoreDirectiveenum to handle ignore directives more cleanly.For easier review, I have split the commits.
Once the review is complete, I will squash and force push them before merging. 🙂