-
Notifications
You must be signed in to change notification settings - Fork 1
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
SDKS-3395 - Align naming and errors for Davinci #4
Conversation
120371a
to
8cc16d1
Compare
8cc16d1
to
3473a77
Compare
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.
Minor change
} | ||
|
||
// Treat unknown status codes as unrecoverable failures | ||
return FailureNode(cause: ApiError.error(status, json, message)) |
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.
Consider failed when return code 2XX?
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.
fixed
3473a77
to
313c59e
Compare
313c59e
to
8f275ff
Compare
8f275ff
to
2130ddd
Compare
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.
Overall looks good, some minor comments
Can we also update the readme file if applicable
@@ -11,7 +11,7 @@ | |||
import PingOrchestrate | |||
|
|||
/// Extension to get the id of a Connector. |
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.
update documentation to reflect the new name
} | ||
|
||
/// Protocol for Node. Represents a node in the workflow. | ||
public protocol Node {} | ||
|
||
public struct EmptyNode: Node { | ||
public init() {} | ||
public init() {} | ||
} | ||
|
||
/// Represents an error node in the workflow. | ||
/// - property cause: The cause of the 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.
update documentation to reflect the new name
|
||
public let input: [String: Any] | ||
public let message: String | ||
public struct ErrorNode: Node { |
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.
update documentation to reflect the new name
static let status = "status" | ||
static let authorizeResponse = "authorizeResponse" | ||
static let code = "code" | ||
static let codeNumber = 1999 |
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 the name reflect the value? code_1999 or something like that
static let connectorId = "connectorId" | ||
static let capabilityName = "capabilityName" | ||
static let requestTimedOut = "requestTimedOut" | ||
static let pingOneAuthConnector = "pingOneAuthenticationConnector" |
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.
to be consistent, I'd suggest to name the variable exactly like the value where possible to avoid any duplicates or confusion later.
All comments addressed |
2130ddd
to
484813a
Compare
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.
Changes looks good to me
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 you also update the README.md file with the new names...?
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.
Code changes look good. However we need to update the
README file for DaVinci module to reflect the new names.
484813a
to
27d8636
Compare
updated read me |
27d8636
to
3ad99b7
Compare
3ad99b7
to
816ac8f
Compare
JIRA Ticket
SDKS-3395 DaVinci Errors and naming