#2435 - Make modifier_order rule rely on an explicit set of rules #2458
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2458 +/- ##
=========================================
+ Coverage 90.67% 90.7% +0.03%
=========================================
Files 322 322
Lines 16225 16261 +36
=========================================
+ Hits 14712 14750 +38
+ Misses 1513 1511 -2
Continue to review full report at Codecov.
|
| public struct ModifierOrderRule: ASTRule, OptInRule, ConfigurationProviderRule { | ||
| public var configuration = ModifierOrderConfiguration(preferredModifierOrder: [.override, .acl]) | ||
| public var configuration = ModifierOrderConfiguration( | ||
| preferredModifierOrder: [ |
There was a problem hiding this comment.
Leaving the order as is would fix all of the violations where some other modifier preceded or was in-between the [.override, .acl] modifiers. Matching the prior behavior of the rule with this version was impossible so I just used the oss-check as a guideline to establish the modifier order. The order should definitely be made less strict if this change is desired.
| @@ -14,163 +25,8 @@ public struct ModifierOrderRule: ASTRule, OptInRule, ConfigurationProviderRule { | |||
| description: "Modifier order should be consistent.", | |||
| kind: .style, | |||
| minSwiftVersion: .fourDotOne , | |||
| nonTriggeringExamples: [ | |||
There was a problem hiding this comment.
can this be its. own commit? hard to see if any of the test cases changed
There was a problem hiding this comment.
Sure, moved to a separate commit. None of the test cases were changed
There was a problem hiding this comment.
Can you add some non triggering test cases like the ones you described above?
There was a problem hiding this comment.
Added the examples
abdulowork
force-pushed
the
modifier-order-fix
branch
2 times, most recently
from
f762335 to
2c5fdd4
Compare
…order specified to conclude a violation
jpsim
force-pushed
the
modifier-order-fix
branch
from
2c5fdd4 to
0deadd1
Compare
|
Thank you very much for this PR @Biboran! I had concerns about this when the rule was originally added, so it's nice to see you address them 😄 I made some small changes and added a changelog entry. I'll merge when CI is happy. |
realm#2458) * realm#2435 - Adjust modifier_order rule to require explicit modifier order specified to conclude a violation * realm#2435 - Move modifier order rule examples to a separate file * realm#2435 - Add modifier interference tests * realm#2435 - Fix whitespaces * Minor edits * Add changelog entry
After a bit of research resulting from #2435 it was found that
modifier_orderrule only cares about specified exact leading ordering. For example given the ordering[.final, .override, .acl]the following would be considered triggering:but this would be considered fine:
Such behavior is not always desired. It might be more beneficial if
modifier_orderrule only enforced order between the specified modifiers and left the unspecified in place. I slightly modified the algorithm and adjusted it such that it's a bit stricter than the currentoss-check.