style(editor): clear the vendored editor packages' lint violations - #2885
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The two vendored editor packages carried 502 SwiftLint violations, because
.swiftlint.ymlhas never coveredLocalPackages/. They are about to move underPackages/, where #2882 already put the gate, so clearing them first keeps that move a pure move rather than a move plus 502 unrelated edits.Eighth step of the editor adoption work, after #2873, #2875, #2877, #2878, #2880, #2881, #2882 and #2883. The config itself does not change here; these files come under the gate when they move.
swiftlint --fixis not safe at this scaleI ran it once for the mechanical 353 and had to repair its output four times. Recording it here because the failures are not the kind a reviewer expects:
public var isEmpty: Bool { count == 0 }intopublic var isEmpty: Bool { isEmpty }inTextLineStorage. That compiles, and recurses until the stack runs out. Nothing but a test that callsisEmptywould ever catch it.empty_countrewrite to three types that have acountand are not Collections:RangeStore, an optional (x?.count == 0becamex?.isEmpty, which isBool?and does not satisfy#expect), and, earlier in chore(lint): lint TablePro's own SwiftPM packages #2882, a tuple(query: String, count: Int).lower_acl_than_parentdroppedpublicfrom anOptionSetnested in anextension NSBezierPath, after whichinit(rawValue: Corners.RawValue)resolvedCornersto AppKit's own nested type and stopped compiling. Naming the raw type (Int) fixes it.--fixapplies correctable rules thatlint --strictdoes not report. Seven app files were reverted.The remaining 149 were done by hand. The trap that hides all of this:
xcodebuildon the app scheme does not compile every package target, so the app build stays green while a package is broken. Both package suites are the gate that matters.Two things this surfaced that are worth reading
A test had been disabled with an underscore and nobody noticed.
identifier_namefired on_test_updateOnResizeNoScrollViewinVisibleRangeProviderTests. The leading underscore is how you stop XCTest collecting a method, so it had not run since it was written, and it asserted nothing: it capturedvisibleSet, resized, and compared. It is nowtest_noScrollViewMakesTheWholeDocumentVisible, it asserts the contract the name claims, and it runs. That is the XCTest count going from 111 to 112.TextView._undoManageris noweditorUndoManager.identifier_namefired on the leading underscore. It is public and the app's tests read it, so the rename and its one caller inEditorLifecycleTeardownTestsare in this commit, per the atomic-API rule.Directives
Four, each because the rule is wrong for that line:
TextLineStorage.isEmpty,empty_count. The rule wantscount == 0written asisEmpty, on the line that definesisEmpty. That is the infinite recursion above, and the directive is what stops the next person's--fixfrom reintroducing it.loadView()overrides,prohibited_super_call.NSViewController.loadView()documents that an override which does not use a nib must setself.view, and these build their view hierarchy on top of the superclass's. Removing thesupercall empties the view.storage_environment_defaults. A package cannot import the app target, so it cannot reachAppStorageEnvironment; the value is a defaulted parameter the host overrides.Verification
swiftlint lint --strictover both packages: 502 to 0. Run through a config that addsLocalPackagestoincluded:, since the root config does not cover them yet; that file is not committed.swift test --package-path LocalPackages/CodeEditTextView --force-resolved-versions: 333 of 333.swift test --package-path LocalPackages/CodeEditSourceEditor --force-resolved-versions: 112 XCTest executed, 2 skipped, 0 failures; Swift Testing 102 of 102.