SwiftLintBot
commented
6 months ago Generated by :no_entry_sign: Danger
Closed mildm8nnered closed 6 months ago
SwiftLintBot
commented
6 months ago Generated by :no_entry_sign: Danger
mildm8nnered
commented
6 months ago Adopted your suggestions pretty much whole-sale - definitely looks much better now
mildm8nnered
commented
6 months ago Ah - I think maybe that period should not be on the end of the reason https://github.com/realm/SwiftLint/pull/5518/commits/7039fba188cff7cdab5c24f7b036dd39e82e5263
https://github.com/realm/SwiftLint/pull/5475 is now failing CI with
Test Suite 'BaselineTests' started at 2024-04-04 17:49:36.549.
Test Case '-[SwiftLintFrameworkTests.BaselineTests testNewViolation]' started.
Reasons shall not end with a period. Got "`swiftlint:disable` commands should use `next`, `this` or `previous` to disable rules for a single line, or `swiftlint:enable` to re-enable the rules immediately after the violations to be ignored, instead of disabling the rule for the rest of the file.". Either rewrite the rule's description or set a custom reason in the StyleViolation's constructor.: file StyleViolation.swift, line 46Hmm. Looks like some rule descriptions do have a period, and some don't. I think that test is failing because I'm creating a BlanketDisableCommand violation without a specific reason in the test, and it's picking up the description - I can probably work around that.
SimplyDanny
commented
6 months ago Ups ... 😬 There are a few things to explain here.
One is that without a specific reason for a violation, the description is used. At the same time, the description appears in the documentation for every rule.
It is reasonable to prefer complete sentences in descriptions and explanations, but we also once agreed on violation messages to be as short as possible and without a full stop to mimic what other tools usually do.
Combining both, there is an unfortunate conflict that can only be resolved by using explicit reasons everywhere or enhance every rule description with a default reason distinct from the description.
That said, sorry for breaking tests.
mildm8nnered
commented
6 months ago Ups ... 😬 There are a few things to explain here.
One is that without a specific reason for a violation, the description is used. At the same time, the description appears in the documentation for every rule.
It is reasonable to prefer complete sentences in descriptions and explanations, but we also once agreed on violation messages to be as short as possible and without a full stop to mimic what other tools usually do.
Combining both, there is an unfortunate conflict that can only be resolved by using explicit reasons everywhere or enhance every rule description with a default reason distinct from the description.
That said, sorry for breaking tests.
no worries - I just switched to a different rule for my tests for now.
I think this particular failure is just because of the way I was using the rule in my tests - users would never see the description as a reason in this case.
Updates the rule description and a reason for the
blanket_disable_rule, avoiding language about the end of the file.Partly addresses #5450
New top level rule description (I think this will only appear in the documentation perhaps).
old: "swiftlint:disable commands should be re-enabled before the end of the file"
new: "
swiftlint:disablecommands should usenext,thisorpreviousto disable rules for a single line, orswiftlint:enableto re-enable the rules immediately after the violations to be ignored, instead of disabling the rule for the rest of the file"and the reason:
old: "The disabled 'XXXX' rule should be re-enabled before the end of the file"
new: "Use 'next', 'this' or 'previous' instead to disable the 'XXXX' rule once or re-enable it as soon as possible"
The OSS violations and fixes should match exactly - they've just been triggered by the reason change.