-
Notifications
You must be signed in to change notification settings - Fork 44
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
[feat]: adopt swift-custom-dump in some testing utilities #317
[feat]: adopt swift-custom-dump in some testing utilities #317
Conversation
- Before: assertStateModifications was difficult to use because the diffs for failed assertions were difficult to read - After: Incorporate CustomDump into assertStateModifications to make the assertions easy to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this change 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like a reasonable inclusion
Package.swift
Outdated
@@ -64,6 +64,7 @@ let package = Package( | |||
.package(url: "https://github.com/pointfreeco/swift-identified-collections", from: "1.0.0"), | |||
.package(url: "https://github.com/pointfreeco/swift-macro-testing", from: "0.4.0"), | |||
.package(url: "https://github.com/pointfreeco/swift-perception", from: "1.1.4"), | |||
.package(url: "https://github.com/pointfreeco/swift-custom-dump", from: "1.3.3"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason this version was picked? will it be compatible with the resolved values used elsewhere (i'm assuming yes, but might as well check).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. Register has v1.2.1 which uses the now-deprecated XCTAssertNoDifference. Good callout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit to downgrade. let me know if you prefer that approach. I'm ambivalent about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
downgrading to match other clients seems reasonable to me. i haven't looked at what changed or why it's deprecated so don't have a strong opinion either.
_ modifications: (inout WorkflowType.State) throws -> Void, | ||
fileID: StaticString = #fileID, | ||
filePath: StaticString = #filePath, | ||
line: UInt = #line, | ||
_ modifications: (inout WorkflowType.State) throws -> Void | ||
column: UInt = #column |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be done in a way that is purely additive so this isn't technically a source break (and thus should require a major semver bump)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah we should probably create a new function and deprecate this one. Though it would be a lot easier to use v1.2.1 for now and handle upgrading later (or use the deprecated function here).
cd0218a
to
3586399
Compare
3586399
to
fd23f6a
Compare
d637515
to
07fbbca
Compare
07fbbca
to
d60d294
Compare
Demo
Complex Demo
Checklist