possible suggestions, based on a fix to our app #555
Conversation
git push
There was a problem hiding this comment.
Hey thanks for opening this. We have a pretty much complete rewrite happening on the next branch and one of the explicit goals is improving logging. I think we will formally adopt tracing and think quite seriously about the devex debugging serialization issues.
|
cool! I'm curious, what was the big pull for building async-stripe? Who's using it? |
async-stripe is a fork from the original project stripe-rs and is currently the only maintained Stripe client bindings for Rust, as Stripe does not officially support the language. All the same kinds of projects and organizations (in other language communities) which use Stripe are the kinds of users async-stripe sees! |
|
cool. |
|
Hey, sorry been a busy few weeks between work and trying to move. Thank you for taking the time here. I am happy to merge these into the current live branch, will make some comments. |
| @@ -102,7 +102,40 @@ impl TokioClient { | |||
| Box::pin(async move { | |||
| let bytes = send_inner(&client, request, &strategy).await?; | |||
| let json_deserializer = &mut serde_json::Deserializer::from_slice(&bytes); | |||
| serde_path_to_error::deserialize(json_deserializer).map_err(StripeError::from) | |||
|
|
|||
| match serde_path_to_error::deserialize(json_deserializer) { | |||
There was a problem hiding this comment.
I would like to, instead, integrate tracing into this crate properly. If you have the time to set this up, some TRACE level statements in the event of a failure would probably be well received.
| @@ -208,7 +208,7 @@ macro_rules! def_id { | |||
| && !s.starts_with($alt_prefix) | |||
| )* { | |||
| // N.B. For debugging | |||
| eprintln!("bad id is: {} (expected: {:?}) for {}", s, $prefix, stringify!($struct_name)); | |||
| // eprintln!("bad id is: {} (expected: {:?}) for {}", s, $prefix, stringify!($struct_name)); | |||
There was a problem hiding this comment.
Same here, it's silly I have left this eprintln for so long. Let's do a trace / debug level log.
| @@ -170,7 +170,7 @@ pub struct Charge { | |||
| /// | |||
| /// It contains the Source, Card, or BankAccount object used for the charge. | |||
| /// For details about the payment method used for this charge, refer to `payment_method` or `payment_method_details` instead. | |||
| pub source: Option<PaymentSource>, | |||
| // pub source: Option<PaymentSource>, | |||
There was a problem hiding this comment.
I don't think this should be commented out
There was a problem hiding this comment.
what's the source of truth that you would use to check this?
This was causing a failure till I commented it out
| @@ -39,3 +39,69 @@ fn is_charge_expandable() { | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
| /* | |||
| Use this test to fix deserialization | |||
There was a problem hiding this comment.
I like it! perhaps a binary for this would be useful, rather than a test :)
|
@arlyon |
|
That is OK with me, I don't mind splitting it up |
Summary
@arlyon
I'm not opening this PR to suggest you merge these changes, as they might not work for everyone, and I don't know all that much about the library, but I will say that this
sourceproperty appears to be deprecated, and that not having it there caused a bug in the case of my application, when a Charge with a source that came from a connected app (Gumroad) came through the pipes.Feel free to close after checking it out?
Checklist
cargo make fmt