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

[Swift 4.2] ReflectionDecodable extension for CaseIterable enums #152

Merged
merged 8 commits into from May 31, 2018

Conversation

@bre7
Copy link
Contributor

bre7 commented May 24, 2018

Depends on Swift 4.2 (shim could be added for Swift <= 4.1)

Closes #148

Checklist

  • Circle CI is passing (code compiles and passes tests).
  • There are no breaking changes to public API.
  • New test cases have been added where appropriate.
  • All new code has been commented with doc blocks ///.

@bre7 bre7 changed the title Added free enum conformance for ReflectionDecodable [Swift 4.2] Added free enum conformance for ReflectionDecodable May 24, 2018

@bre7 bre7 changed the title [Swift 4.2] Added free enum conformance for ReflectionDecodable [Swift 4.2] ReflectionDecodable extension for CaseIterable enums May 25, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

LGTM, just one comment.

)
}
/// cases should be different
guard let first = allCases.first, let last = allCases.last else {

This comment has been minimized.

@tanner0101

tanner0101 May 30, 2018

Member

You should be able to combine these two guard statements to prevent needing to dupe the error message.

This comment has been minimized.

@bre7

bre7 May 30, 2018

Author Contributor

Opted for verbosity, but I'll change them if you prefer a single guard 😉

@tanner0101 tanner0101 self-assigned this May 30, 2018

@tanner0101 tanner0101 added this to the 3.2.2 milestone May 30, 2018

bre7 and others added some commits May 30, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

We should add a Swift 4.2 test to the circle.yml file to ensure the new code is passing tests.

/// See `ReflectionDecodable.reflectDecoded(_:)` for more information.
public static func reflectDecoded() throws -> (Self, Self) {
/// enum must have at least 2 unique cases
guard !allCases.isEmpty, allCases.count > 1,

This comment has been minimized.

@tanner0101

tanner0101 May 30, 2018

Member

Checking that the array is not empty and checking the count is redundant.

This comment has been minimized.

@bre7

bre7 May 30, 2018

Author Contributor

I know but isEmpty is more efficient though (enums are rarely big so I guess it doesn't really matter)

func testCaseIterableExtension() throws {
#if swift(>=4.2)
enum Pet: String, CaseIterable, ReflectionDecodable, Decodable {
case cat

This comment has been minimized.

@tanner0101

tanner0101 May 30, 2018

Member

How is this test working if there is only one case?

This comment has been minimized.

@bre7

bre7 May 30, 2018

Author Contributor

Oops, deleted to check for fail but forgot to add it back 😨

This comment has been minimized.

@calebkleveter

calebkleveter May 30, 2018

Member

You might want to have 2 enums, you to test for failure.

This comment has been minimized.

@bre7

bre7 May 30, 2018

Author Contributor

👍 Yeah, added

bre7 added some commits May 30, 2018

@bre7

This comment has been minimized.

Copy link
Contributor Author

bre7 commented May 30, 2018

Why does the following happen ? (type is Pet enum)

  • [Swift 4.1] "\(type)" = "EnumName #1"
  • [Swift 4.2] "\(type)" = "EnumName"

Reason why some tests fail when using Swift 4.2

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented May 31, 2018

Seems like a difference in how Swift is serializing names. Going to merge this and fix the tests on master, thanks!

@tanner0101 tanner0101 merged commit 43e5ed8 into vapor:master May 31, 2018

8 of 9 checks passed

ci/circleci: linux Your tests failed on CircleCI
Details
ci/circleci: linux-fluent-mysql Your tests passed on CircleCI!
Details
ci/circleci: linux-fluent-postgresql Your tests passed on CircleCI!
Details
ci/circleci: linux-fluent-sqlite Your tests passed on CircleCI!
Details
ci/circleci: linux-jwt Your tests passed on CircleCI!
Details
ci/circleci: linux-leaf Your tests passed on CircleCI!
Details
ci/circleci: linux-redis Your tests passed on CircleCI!
Details
ci/circleci: linux-release Your tests passed on CircleCI!
Details
ci/circleci: linux-vapor Your tests passed on CircleCI!
Details
@penny-coin

This comment has been minimized.

Copy link

penny-coin commented May 31, 2018

Hey @bre7, you just merged a pull request, have a coin!

You now have 4 coins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment