Skip to content

Conversation

@curiousepic
Copy link
Contributor

In preparation to rename FirstdataE4V27 to FirstdataE4, making the names
accurately refer to the support gateway versions.

Unit:
32 tests, 157 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

Remote:
No currently working test creds.

In preparation to rename FirstdataE4V27 to FirstdataE4, making the names
accurately refer to the support gateway versions.

Unit:
32 tests, 157 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
@curiousepic curiousepic requested a review from a team March 5, 2019 21:15
@curiousepic
Copy link
Contributor Author

Inquiring about current test creds, but thought I'd throw this up for early scrutiny.

@curiousepic
Copy link
Contributor Author

We would also want to cut a minor version before merging this.

Copy link
Contributor

@jknipp jknipp left a comment

Choose a reason for hiding this comment

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

Looks good so far.

Tangential question, when would it make sense to reuse code through inheritance? The v11 and v27+ versions of the adapter share much of the same code. In what cases should or shouldn't we have a base adapter that the other adapters inherit from?

@curiousepic
Copy link
Contributor Author

curiousepic commented Mar 6, 2019

@jknipp Good question; we could certainly have taken that path when implementing v28, but it probably doesn't make sense to move to it retroactively in this case. That said, I'm not sure if there are any instances of inheritance strictly for versioning of the same API; I think more often there are common core elements used for multiple endpoints with different functionality.

Edit: Quickpay does do this for versioning.

@jknipp
Copy link
Contributor

jknipp commented Jun 10, 2019

@curiousepic Are we ok to close this?

@curiousepic
Copy link
Contributor Author

@jknipp This fell through the cracks, though we might still be waiting on test creds for confirmation? In any case it's something that should still happen.

@curiousepic
Copy link
Contributor Author

Marking this "of interest" before a cleanup of stale PRs

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants