Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Multiple Fixes for custom rules #3473

Merged
merged 7 commits into from
Jan 15, 2021
Merged

Conversation

fredpi
Copy link
Collaborator

@fredpi fredpi commented Dec 18, 2020

Addresses #3472.

The reason why this issue occurred is that I didn't properly apply the new principle of first defining all available rules and only then filtering those that are active depending on the mode to the custom rules. As you can see, I now moved the custom rules filtering logic to the same place where the normal rules get filtered so that the filtering isn't part of the custom rules merging algorithm.

@marcelofabri
Copy link
Collaborator

marcelofabri commented Dec 18, 2020

Can you please add a test that would fail without this change so we avoid regressions in the future? 🙏

In the mean time, I'll test it against my project to confirm the fix

UPDATE: Confirmed that it fixed the issue for me.

@fredpi
Copy link
Collaborator Author

fredpi commented Dec 18, 2020

@marcelofabri I thought about adding a test, but that would require me to leak some currently private interfaces of the RulesWrapper class across multiple layers. That may be worth it but I want to have a look at #3468 first to find out if an access level modification is needed anyway. I hope to find some time for that tomorrow.

@fredpi
Copy link
Collaborator Author

fredpi commented Dec 18, 2020

Linux tests failing because of a new Swift version... I guess this shouldn't get fixed within this PR.

@SwiftLintBot
Copy link

SwiftLintBot commented Dec 18, 2020

12 Messages
📖 Linting Aerial with this PR took 2.02s vs 2.01s on master (0% slower)
📖 Linting Alamofire with this PR took 2.83s vs 2.84s on master (0% faster)
📖 Linting Firefox with this PR took 9.5s vs 9.5s on master (0% slower)
📖 Linting Kickstarter with this PR took 15.47s vs 15.33s on master (0% slower)
📖 Linting Moya with this PR took 1.36s vs 1.38s on master (1% faster)
📖 Linting Nimble with this PR took 1.31s vs 1.29s on master (1% slower)
📖 Linting Quick with this PR took 0.65s vs 0.59s on master (10% slower)
📖 Linting Realm with this PR took 3.94s vs 3.95s on master (0% faster)
📖 Linting SourceKitten with this PR took 1.03s vs 1.02s on master (0% slower)
📖 Linting Sourcery with this PR took 7.72s vs 7.68s on master (0% slower)
📖 Linting Swift with this PR took 11.04s vs 11.16s on master (1% faster)
📖 Linting WordPress with this PR took 17.51s vs 17.63s on master (0% faster)

Generated by 🚫 Danger

@fredpi
Copy link
Collaborator Author

fredpi commented Dec 19, 2020

This PR also addresses #3468 now.

@marcelofabri I' rather not add the test you suggested as I can only think of a way of implementing it that would be depending largely on the implementation rather than on the result.

@fredpi fredpi force-pushed the feature/3472-fix-custom-rules-merging branch from ba17d60 to 30b35bf Compare December 21, 2020 11:41
@fredpi fredpi changed the title Separate custom rules merging & filtering to avoid misleading warnings Multiple Fixes for custom rules Dec 22, 2020
@fredpi
Copy link
Collaborator Author

fredpi commented Dec 22, 2020

Now also addresses #3477.

@fredpi
Copy link
Collaborator Author

fredpi commented Jan 12, 2021

As these are quite important fixes, I will test this again and probably merge it in some days' time, unless concerns emerge in the meantime.

@fredpi fredpi force-pushed the feature/3472-fix-custom-rules-merging branch from 90645a1 to 2d17045 Compare January 14, 2021 22:35
@fredpi fredpi merged commit 537e53f into master Jan 15, 2021
@fredpi fredpi deleted the feature/3472-fix-custom-rules-merging branch January 15, 2021 10:53
jpsim added a commit that referenced this pull request Jan 22, 2021
…ules-merging"

This reverts commit 537e53f, reversing
changes made to ba49f7d.
jpsim added a commit that referenced this pull request Jan 22, 2021
…ules-merging" (#3503)

This reverts commit 537e53f, reversing
changes made to ba49f7d.
DwayneCoussement pushed a commit to DwayneCoussement/SwiftLint that referenced this pull request Feb 1, 2021
commit bbf1ad4
Author: JP Simard <jp@jpsim.com>
Date:   Fri Jan 29 13:59:28 2021 -0500

    [ExplicitSelfRule] Fix violation location and misplaced corrections (realm#3507)

commit 59eb887
Author: Otavio Cordeiro <otaviocc@users.noreply.github.com>
Date:   Fri Jan 29 18:30:57 2021 +0100

    Add opt-in rule discouraged_assert (realm#3506)

    Co-authored-by: Keith Smiley <keithbsmiley@gmail.com>

commit 6a38b15
Author: JP Simard <jp@jpsim.com>
Date:   Fri Jan 22 13:31:05 2021 -0500

    Revert "Merge pull request realm#3473 from realm/feature/3472-fix-custom-rules-merging" (realm#3503)

    This reverts commit 537e53f, reversing
    changes made to ba49f7d.

commit 6de5771
Author: JP Simard <jp@jpsim.com>
Date:   Fri Jan 22 12:44:30 2021 -0500

    [Analyze] Support compile commands with relative paths (realm#3501)

    * [Analyze] Support compile commands with relative paths

    * Add changelog entry

    And made small formatting edits to other recent entries.

commit 537e53f
Merge: ba49f7d 22d25da
Author: Frederick Pietschmann <19194800+fredpi@users.noreply.github.com>
Date:   Fri Jan 15 11:53:17 2021 +0100

    Merge pull request realm#3473 from realm/feature/3472-fix-custom-rules-merging

    Multiple Fixes for custom rules

commit 22d25da
Author: Frederick Pietschmann <19194800+fredpi@users.noreply.github.com>
Date:   Fri Jan 15 00:14:50 2021 +0100

    Add newline at end of yml test file

commit 2d17045
Author: Frederick Pietschmann <19194800+fredpi@users.noreply.github.com>
Date:   Tue Dec 22 19:41:40 2020 +0100

    [realm#3477] Fix bug that prevented the reconfiguration of a custom rule in a child config

commit a39e72e
Author: Frederick Pietschmann <19194800+fredpi@users.noreply.github.com>
Date:   Mon Dec 21 12:40:53 2020 +0100

    Fix changelog

commit 56e4f17
Author: Frederick Pietschmann <19194800+fredpi@users.noreply.github.com>
Date:   Sun Dec 20 00:52:17 2020 +0100

    Fix if statement formatting

commit 5bffc77
Author: Frederick Pietschmann <19194800+fredpi@users.noreply.github.com>
Date:   Sun Dec 20 00:44:49 2020 +0100

    Fix custom_rules merging when a configuration is based on only_rules

commit e3e169b
Author: Frederick Pietschmann <19194800+fredpi@users.noreply.github.com>
Date:   Sat Dec 19 00:11:13 2020 +0100

    Add changelog entry

commit 256288a
Author: Frederick Pietschmann <19194800+fredpi@users.noreply.github.com>
Date:   Sat Dec 19 00:03:13 2020 +0100

    Separate custom rules merging & filtering to avoid misleading warnings
jpsim added a commit that referenced this pull request Feb 23, 2021
…custom-rules-merging""

This reverts commit ca4a0aa.

Going to try to use this PR to remember to narrow down the issues I saw in my closed source project when these changes were originally merged.
fredpi added a commit that referenced this pull request Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants