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

Deprecate predictable helper methods #233

Merged
merged 3 commits into from
Nov 5, 2021
Merged

Deprecate predictable helper methods #233

merged 3 commits into from
Nov 5, 2021

Conversation

Pimm
Copy link
Collaborator

@Pimm Pimm commented Nov 5, 2021

Deprecated methods/properties such as:

  • payment.isOpen() (synonymous with payment.status == PaymentStatus.open)
  • list.count (synonymous with list.length)
  • error.getMessage() (synonymous with error.message)

Cons

  1. Removing these methods/properties will be a breaking change.
  2. Some of these methods/properties also exist in the PHP client, therefore removing them will also widen the gap between the two clients.

Pros

  1. Consumers of an API expect every part of it to be significant. When a consumer sees that a list has both a length and a count, they might wonder "What is the difference between these two? Perhaps length is the number of elements in this array, where count is the total number of elements available?" Methods/properties with such simple synonyms can cause confusion.
  2. Having a narrower API enhances discoverability. Without isOpen, isAuthorized, isExpired, isFailed, isPending, hasSequenceTypeFirst, and hasSequenceTypeRecurring, it's easier to find getCheckoutUrl and getRefunds. It's easier to find the needle if the haystack is smaller.
  3. Having fewer roads leading to Rome promotes consistency in the code which uses this library.

This includes payment.isOpen (which is synonymous with payment.status == PaymentStatus.open); list.count (which is synonymous with list.length); and error.getMessage (which is synonymous with error.message).
Copy link
Collaborator

@edorivai edorivai left a comment

Choose a reason for hiding this comment

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

LGTM

@Pimm Pimm merged commit 4382cec into master Nov 5, 2021
@Pimm Pimm deleted the pimm/v3.6.0 branch November 5, 2021 13:56
@Pimm Pimm mentioned this pull request Nov 5, 2021
@dsempel
Copy link
Collaborator

dsempel commented Nov 8, 2021

I agree this is better.

This was referenced Nov 15, 2021
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.

3 participants