-
Notifications
You must be signed in to change notification settings - Fork 64
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
Update payment methods #376
Conversation
24b57ea
to
171f581
Compare
171f581
to
d79e4ee
Compare
@janpaepke mentioned that this PR Introduces a breaking change, but I'm unsure whether I agree. The following code compiles before this PR, but not after: mollieClient.payments.create({
method: PaymentMethod.giropay,
…
}); Existing code no longer compiler is the tell-tale of a breaking change. However, while the code above does compile before this PR, it doesn't work anyway, as Giropay has been deprecated. I am currently on the fence whether this change ‒ and similar future changes ‒ should be considered "breaking" and thus warrant a major version bump. |
It was not marked as deprecated in the code and thus a consumer could potentially be unaware. Also using the enum in create isn't the only usecase. Another likely example is a comparison check which will not cause a problem until this PR is merged. Imho in this particular example it's pretty clear that this should be considered "breaking". |
The point that I was trying to make, is that semver is supposed to guarantee that everything continues to function as expected for any non-major update. The second a payment method stops working, existing applications break, even if they don't update this client at all. The semver guarantee doesn't work in that scenario. As such, I am not sure whether it's a bad thing to break the semver rules in such a case. |
50c481d
to
88ad956
Compare
I think I got your point, but as I said, if all they do is checks, the application won't break, because it simply won't receive that payment method anymore... |
Very true, Jan. |
Fixes #357
The reference list is taken from the API docs for the get method.
Added the following new payment methods to
PaymentMethod
:Moved outdated payment methods to
HistoricPaymentMethod
: