-
Notifications
You must be signed in to change notification settings - Fork 572
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
Rename a lot of core classes to match the correct naming scheme of the library #1961
Conversation
@@ -2,7 +2,7 @@ namespace Stripe | |||
{ | |||
using Newtonsoft.Json; | |||
|
|||
public class OutcomeRule : StripeEntity<OutcomeRule>, IHasId | |||
public class ChargeOutcomeRule : StripeEntity<ChargeOutcomeRule>, IHasId |
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.
So. I wanted to flag that one explicitly @ob-stripe.
It's quite an old property but, if I am not mistake, this should be a real API resource with an object
field since this is expandable on Charge.outcome.rule
. Really curious what your thoughts are here?
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.
Strictly speaking we don't need the object
field if the expandable field is not polymorphic. But AFAIK this is the only case of an expandable field where the expanded object is not an API resource with an object
field.
I don't think it's worth fixing, but we should discuss this internally to decide if this is something we'd want to allow on other API resources in the future.
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.
The reason I'm asking is that, if we added object
later the class would have the object value as a name. When right now it's kinda ChargeOutcomeRule
and in a way it's annoying
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.
Do you think we should call this one Rule
in the Radar namespace in that case so that in the future, if we add object
it will likely be object: "radar.rule"
?
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.
We should do the same thing as stripe-java to facilitate future codegen, so yes, ChargeOutcomeRule
should be renamed to Rule
in the Radar namespace: https://github.com/stripe/stripe-java/blob/master/src/main/java/com/stripe/model/radar/Rule.java. (Thanks for checking!)
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.
Fixed, PTAL
50e6851
to
32b877d
Compare
98608dc
to
225b3b8
Compare
@remi-stripe Should we close this for now? |
32b877d
to
bfb0674
Compare
I've reset the |
225b3b8
to
a4fc708
Compare
@ob-stripe Re-assigning to you for review again. Flagging I fixed another issue raised by @clement911 in #1969 (comment) |
d0dc6b3
to
5a6c96a
Compare
f88b3ee
to
28378b3
Compare
Took the opportunity to rename a lot of older classes that were never prefixed, paving the way for the release of the codegen-ed library in the next few months.
List of changes:
Fee
toBalanceTransactionFee
onBalanceTransaction
Outcome
toChargeOutcome
onCharge
Evidence
toDisputeEvidence
andEvidenceDetails
toDisputeEvidenceDetails
onDispute
OutcomeRule
toRule
and moved it to theRadar
namespaceShippingMethod
toOrderShippingMethod
,StatusTransitions
toOrderStatusTransitions
andDeliveryEstimate
toOrderShippingMethodDeliveryEstimate
onOrder
Inventory
toSkuInventory
onSku
NextPendingInvoiceItemInvoice
onSubscription
to be aDateTime?
FraudDetails
onCharge
is now a classChargeFraudDetails
instead of a dictionaryConnect
on theWebhookEndpoint
resource as it is not supportedcc @stripe/api-libraries