Skip to content
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

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Conversation

jeyanthanperiyasamy
Copy link
Contributor

@jeyanthanperiyasamy jeyanthanperiyasamy commented Oct 4, 2024

JIRA Ticket

SDKS-3395 DaVinci Errors and naming

Copy link

@witrisna witrisna left a 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))
Copy link

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Contributor

@vahancouver vahancouver left a 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.
Copy link
Contributor

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.
Copy link
Contributor

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 {
Copy link
Contributor

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
Copy link
Contributor

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"
Copy link
Contributor

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.

@jeyanthanperiyasamy
Copy link
Contributor Author

All comments addressed

Copy link

@rodrigoareis rodrigoareis left a 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

Copy link
Contributor

@spetrov spetrov left a 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...?

Copy link
Contributor

@vahancouver vahancouver left a 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.

@jeyanthanperiyasamy
Copy link
Contributor Author

updated read me

@spetrov spetrov merged commit 59ddf76 into develop Oct 16, 2024
6 checks passed
@spetrov spetrov deleted the SDKS-3395 branch October 16, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants