-
Notifications
You must be signed in to change notification settings - Fork 12
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
Version 0.4.0 #8
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 11 27 +16
Lines 517 614 +97
=====================================
- Misses 517 614 +97
|
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.
One comment, nothing outstanding
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 know this was merged in already but wanted to voice my concerns. Also, tests?
// Created by Francesco Paolo Severino on 30/06/24. | ||
// | ||
|
||
public enum OrdersError: Error { |
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.
Before you release the major version this should be hidden behind a struct as in https://github.com/vapor/jwt-kit/blob/main/Sources/JWTKit/JWTError.swift IMO as adding/removing cases to this (which is likely) is source breaking and requires a new major release
} | ||
|
||
/// The type of transit for a boarding pass. | ||
public enum TransitType: String, Encodable { |
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.
As above
} | ||
|
||
/// The format of the barcode. | ||
public enum BarcodeFormat: String, Encodable { |
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.
As above
import Foundation | ||
|
||
public enum PassKitError: Error { | ||
public enum PassesError: Error { |
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.
As above
var _$order: Parent<OrderType> { | ||
guard let mirror = Mirror(reflecting: self).descendant("_order"), | ||
let order = mirror as? Parent<OrderType> else { | ||
fatalError("order property must be declared using @Parent") |
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 a big fan of these fatalError
s honestly
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.
What could I replace them with? In most cases I cannot make these properties optional
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.
Throwing custom Error
s which can be caught (and debugged) and don't just crash the app
Thanks for the feedback Paul! |
PKErrorLog
model, where thecreated_at
FieldKey
didn't coincide in the model and in the migration.authenticationToken
unique for each pass, as requested by the Wallet documentation, improving security.PassKit
for the types shared between thePasses
and the upcomingOrders
modules.PassJSON
protocol to help build thepass.json
.Orders
target.