-
Notifications
You must be signed in to change notification settings - Fork 756
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
Remove legacy parameter support in invoices.retrieveUpcoming() #621
Conversation
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.
Flagged a minor comment otherwise LGTM but deferring to alex.
lib/resources/Invoices.js
Outdated
urlParams: ['id'], | ||
}), | ||
|
||
listLinesUpcoming: stripeMethod({ |
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.
In other libraries we do this by passing upcoming
as the id.
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.
I would actually think this'd be listUpcomingLines
...
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.
I'm also uncomfortable-but-peaceful with an 'upcoming'
id if that's what we do in other libs (and if it has not caused user confusion/sadness)
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.
I renamed it listUpcomingLineItems
, which is consistent with what we do in .NET: https://stripe.com/docs/api/invoices/upcoming_invoice_lines?lang=dotnet.
We should also add this to other languages because having users pass a magic ID as a string is terrible.
818a0ec
to
e2600a2
Compare
Thanks for doing this! ptal @ob-stripe |
e2600a2
to
0e281ad
Compare
@remi-stripe @rattrayalex-stripe Are you both okay with |
I am okay with that! LGTM Thanks for this ob! |
Cool. I updated the WIP migration guide with the changes here. Pulling this in! |
r? @rattrayalex-stripe @remi-stripe
cc @stripe/api-libraries @irace-stripe
Removes support for legacy customer and subscription parameters in
invoices.retrieveUpcoming()
. All parameters must now be provided in a hash.While I was at it, I also noticed that
retrieveLines()
should really be calledlistLineItems()
because it returns a list object containing a list ofline_item
s.I also added
listUpcomingLineItems()
because I think that's much cleaner than telling users to uselistLines()
with"upcoming"
as the ID.