Enum namespace rule #737
Enum namespace rule #737
Conversation
facumenzella
force-pushed
the
convenience-type
branch
4 times, most recently
from
5cbe1bf
to
a6765d3
Compare
- Added test suite
facumenzella
force-pushed
the
convenience-type
branch
from
a6765d3
to
82831d2
Compare
| var j = startIndex | ||
| while j < endIndex, let token = formatter.token(at: j) { | ||
| // exit if there's a explicit init | ||
| if token == .keyword("init") { |
There was a problem hiding this comment.
It seems like this implementation may run into the same issues as #728. This should probably only check declarations at the top level of the type definition.
Some examples that I think may get handled incorrectly right now:
struct Namespace {
static func staticFunction() {
func nestedFunction() { /* ... */ }
}
}struct Namespace {
static func staticFunction() {
struct NestedType {
init() {}
}
}
}struct Namespace {
struct TypeNestedInNamespace {
let foo: Int
let bar: Int
}
}struct Namespace {
struct NestedNamespace {
static let foo: Int
static let bar: Int
}
}You could potentially use parseDeclarations() to get the list of top-level declarations for each type (this could make it easier to handle recursively nested types as well).
There was a problem hiding this comment.
Thanks for the review!!
I'll add some tests for that and fix them.
Rules.md
Outdated
| @@ -9,6 +9,7 @@ | |||
| * [braces](#braces) | |||
| * [consecutiveBlankLines](#consecutiveBlankLines) | |||
| * [consecutiveSpaces](#consecutiveSpaces) | |||
| * [convenienceType](#convenienceType) | |||
There was a problem hiding this comment.
I know the SwiftLint rule is also called convenienceType, but I'm not really sure what "convenience" means in this context. I've always heard caseless enums referred to as namespaces. I think a name like enumNamespaces could make sense here.
Tests/RulesTests+Braces.swift
Outdated
| @@ -57,7 +57,7 @@ extension RulesTests { | |||
| // foo | |||
| } | |||
| """ | |||
| testFormatting(for: input, rule: FormatRules.braces) | |||
| testFormatting(for: input, rule: FormatRules.braces, exclude: ["convenienceType"]) | |||
There was a problem hiding this comment.
I noticed that the SwiftLint rule doesn't trigger for empty types like class Foo {}. That may be a nice addition to this implementation (and would help avoid the need to exclude this rule from so many existing tests)
facumenzella
force-pushed
the
convenience-type
branch
from
291e31c
to
e74948f
Compare
facumenzella
force-pushed
the
convenience-type
branch
2 times, most recently
from
1a4f1c3
to
faa84fe
Compare
facumenzella
force-pushed
the
convenience-type
branch
from
faa84fe
to
cb9c02d
Compare
| @@ -3,7 +3,7 @@ | |||
| import Foundation | |||
|
|
|||
| /// A protocol for view-type classes that can be configured using Layout | |||
| @objc protocol LayoutConfigurable: class { | |||
| @objc protocol LayoutConfigurable: enum { | |||
There was a problem hiding this comment.
@facumenzella I didn't spot this before, but this looks like a bug. I'd guess the rule wrongly identified this as a class whereas in fact it's a protocol.
There was a problem hiding this comment.
Oh I completely missed this one. I thought it was under control. I'll take a look
There was a problem hiding this comment.
@facumenzella don't worry, I checked and the rule itself is fine (you added a unit test for this case) - I think you must've just forgotten to roll back this change to the snapshot tests.
Adopt new helpers in organizeDeclarations Make tests pass Add another test case Add extensionacl option to place the ACL keyword on the extension or on the body declarations Fix unit test line assignment Fix spurious removal of type in ternary assignment Updated for 0.46.3 release Simplify example Add more test cases Updated for 0.46.3 release Enum namespace rule (nicklockwood#737)
Why?
What?
This PR tries to tackle an existing rule from Swiftlint. Types used to host static members should be enums to avoid instantiation.