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

Add "src" as a valid prefix for PaymentMethodId #164

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

andrewhalle
Copy link
Contributor

No description provided.

@FL33TW00D
Copy link
Contributor

@andrewhalle can you link the appropriate section of the API docs for this change?

@arlyon arlyon added the question Further information is requested label Feb 10, 2022
@andrewhalle
Copy link
Contributor Author

@FL33TW00D the Sources API page is listed in the "Payment Methods" section, is there a more specific place I can reference? I noticed this when a PaymentIntent I was testing with failed to de-serialize. Going by the pages listed under "Payment Methods", I suppose I could also add ba as a valid prefix for bank accounts, if you agree.

@FL33TW00D
Copy link
Contributor

@FL33TW00D the Sources API page is listed in the "Payment Methods" section, is there a more specific place I can reference? I noticed this when a PaymentIntent I was testing with failed to de-serialize. Going by the pages listed under "Payment Methods", I suppose I could also add ba as a valid prefix for bank accounts, if you agree.

Looks good, ba and src both seem to be valid prefixes. Thanks for the link

@andrewhalle
Copy link
Contributor Author

No problem! I'll add that. Any preference on if I also add a trailing underscore to pm and card? PaymentMethodId seems to be the only def_id that doesn't follow that style.

* add "ba" as a valid prefix for PaymentMethodId
* add trailing underscores for the valid prefixes of PaymentMethodId to
  keep with style of other `def_id!`s
@FL33TW00D
Copy link
Contributor

No problem! I'll add that. Any preference on if I also add a trailing underscore to pm and card? PaymentMethodId seems to be the only def_id that doesn't follow that style.

Based on the rest of the ID's (

def_id!(PersonId, "person_");
), I'd be happy with a trailing prefix. Any thoughts @arlyon?

@arlyon
Copy link
Owner

arlyon commented Feb 11, 2022

Hey guys thanks for raising this discussion @andrewhalle! I am going to get in touch with people at stripe and get a canonical list of all id formats so that we ensure these are all correct! Stay tuned :)

@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #164 (e626d29) into master (b14126e) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #164      +/-   ##
=========================================
- Coverage    5.09%   5.03%   -0.06%     
=========================================
  Files         117     117              
  Lines        8836    8841       +5     
=========================================
- Hits          450     445       -5     
- Misses       8386    8396      +10     
Impacted Files Coverage Δ
src/ids.rs 34.31% <ø> (ø)
src/resources/generated/token.rs 0.00% <0.00%> (ø)
src/resources/generated/charge.rs 0.77% <0.00%> (ø)
src/resources/generated/payout.rs 0.00% <0.00%> (ø)
src/resources/generated/refund.rs 0.00% <0.00%> (ø)
src/resources/generated/transfer.rs 0.00% <0.00%> (ø)
src/resources/generated/payment_intent.rs 0.00% <0.00%> (ø)
src/resources/generated/payment_method.rs 0.00% <0.00%> (ø)
src/resources/generated/checkout_session.rs 0.00% <0.00%> (ø)
src/resources/generated/customer.rs 21.05% <0.00%> (+0.13%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b14126e...e626d29. Read the comment docs.

@andrewhalle
Copy link
Contributor Author

No problem, thank you!

@arlyon
Copy link
Owner

arlyon commented Feb 15, 2022

I have reached out to stripe, will update with progress. In the mean time, I am happy to merge this. Here is another (incomplete) list: https://gist.github.com/fnky/76f533366f75cf75802c8052b577e2a5

@arlyon arlyon merged commit ccf4c7e into arlyon:master Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants