Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix custom_rules regression introduced in #4755 #5336

Closed
wants to merge 3 commits into from

Conversation

marcelofabri
Copy link
Collaborator

@marcelofabri marcelofabri commented Nov 8, 2023

Here's what was happening:

  • CustomRules.swift filters out violations outside of disabled regions
  • This means that we won't get that violation in Linter so we'll always trigger superfluous_disable_command
  • Region.isRuleEnabled doesn't understand custom rules

We can change the logic a little bit to make it work for custom rules. I don't love this, but I think it works (and we already special case CustomRules in a few places).

@SwiftLintBot
Copy link

SwiftLintBot commented Nov 8, 2023

1 Warning
⚠️ If this is a user-facing change, please include a CHANGELOG entry to credit yourself!
You can find it at CHANGELOG.md.
16 Messages
📖 Linting Aerial with this PR took 1.19s vs 1.19s on main (0% slower)
📖 Linting Alamofire with this PR took 1.56s vs 1.55s on main (0% slower)
📖 Linting Brave with this PR took 8.93s vs 8.95s on main (0% faster)
📖 Linting DuckDuckGo with this PR took 4.5s vs 4.5s on main (0% slower)
📖 Linting Firefox with this PR took 10.44s vs 10.46s on main (0% faster)
📖 Linting Kickstarter with this PR took 11.02s vs 11.01s on main (0% slower)
📖 Linting Moya with this PR took 0.62s vs 0.61s on main (1% slower)
📖 Linting NetNewsWire with this PR took 3.28s vs 3.28s on main (0% slower)
📖 Linting Nimble with this PR took 0.82s vs 0.82s on main (0% slower)
📖 Linting PocketCasts with this PR took 8.84s vs 8.82s on main (0% slower)
📖 Linting Quick with this PR took 0.39s vs 0.4s on main (2% faster)
📖 Linting Realm with this PR took 11.69s vs 11.69s on main (0% slower)
📖 Linting Sourcery with this PR took 2.82s vs 2.81s on main (0% slower)
📖 Linting VLC with this PR took 1.56s vs 1.55s on main (0% slower)
📖 Linting Wire with this PR took 19.65s vs 19.59s on main (0% slower)
📖 Linting WordPress with this PR took 13.36s vs 13.33s on main (0% slower)

Here's an example of your CHANGELOG entry:

* Fix custom_rules regression introduced in #4755.  
  [marcelofabri](https://github.com/marcelofabri)
  [#issue_number](https://github.com/realm/SwiftLint/issues/issue_number)

note: There are two invisible spaces after the entry's text.

Generated by 🚫 Danger

@marcelofabri marcelofabri marked this pull request as ready for review November 8, 2023 05:50
@keith
Copy link
Collaborator

keith commented Nov 8, 2023

Looks like this might not be enough, with this commit I still see the issue, here's another example:

        if UIAccessibility.isVoiceOverRunning { // swiftlint:disable:this use_voice_over_provider
            return
        }
  use_voice_over_provider:
    name: "Use `getVoiceOverProvider()`."
    regex: '\bUIAccessibility\.isVoiceOverRunning\b'
    match_kinds:
      - identifier
      - typeidentifier

@marcelofabri
Copy link
Collaborator Author

took a quick look and couldn't repro it with a unit test:

    func testSuperfluousDisableCommandDoesntTriggerWithCustomRulesDisableThis() throws {
        let customRulesConfiguration: [String: Any] = [
            "use_voice_over_provider": [
                "regex": "\\bUIAccessibility\\.isVoiceOverRunning\\b",
                "match_kinds": ["identifier", "typeidentifier"]
            ]
        ]

        let example = Example(
            """
            if UIAccessibility.isVoiceOverRunning {} // swiftlint:disable:this use_voice_over_provider
            """,
            configuration: customRulesConfiguration
        ).skipWrappingInCommentTest()
        let configuration = try XCTUnwrap(makeConfig(["custom_rules": customRulesConfiguration], "custom_rules"))
        let violations = violations(example, config: configuration)

        XCTAssertEqual(violations.count, 0)
    }

i'll try to dig deeper tonight, but i'm inclined to just revert the PR.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants