file_link support #1244
file_link support #1244
Conversation
|
|
||
| public virtual StripeFileLink Create(StripeFileLinkCreateOptions createOptions, StripeRequestOptions requestOptions = null) | ||
| { | ||
| return CreateAsync(createOptions, requestOptions).Result; |
There was a problem hiding this comment.
I've used a style I'd like to see us adopt for new services (and apply to the existing services):
- Methods are in alphabetical order (like in tests)
- Non-async methods simply call their async version +
.Result, as suggested by @jaymedavis in concept (do not merge): reducing code in the services #940.
There was a problem hiding this comment.
Are we sure it works across all .NET frameworks?
Also, if we approve this, should we convert the whole library once and for all?
There was a problem hiding this comment.
Thanks @ob-stripe for keeping things nice and neat and caring about details. Warms my ❤️
There was a problem hiding this comment.
@remi-stripe The library almost does this anyway on non async calls. Notice it uses the addition of GetAwaiter().GetResult(). I believe this was determined good practice from #1145. This stack overflow post sheds some light about the differences... which makes me wonder if this change was necessary.
There was a problem hiding this comment.
Further reading concludes that it is necessary! You’d get the real exception vs an Aggregate exception. I would change .Result everywhere to GetAwaiter().GetResult()
Source: aspnet/Security#59
| /// <summary> | ||
| /// A future timestamp after which the link will no longer be usable. | ||
| /// </summary> | ||
| public DateTime? ExpiresAt { get; set; } |
There was a problem hiding this comment.
you likely should have a StripeFileLinkSharedOptions class since they have 2 parameters in common. Not a huge deal though since it does work that way
There was a problem hiding this comment.
Ack. I've updated the PR with a StripeFileLinkSharedOptions class.
| { | ||
| using Newtonsoft.Json; | ||
|
|
||
| public class StripeFileLinkListOptions : StripeListOptions |
There was a problem hiding this comment.
Minor but there's a class called StripeListOptionsWithCreated that should either be used everywhere we have created or canned.
There was a problem hiding this comment.
Ack. I've updated the PR to use it.
|
|
||
| public virtual StripeFileLink Create(StripeFileLinkCreateOptions createOptions, StripeRequestOptions requestOptions = null) | ||
| { | ||
| return CreateAsync(createOptions, requestOptions).Result; |
There was a problem hiding this comment.
Are we sure it works across all .NET frameworks?
Also, if we approve this, should we convert the whole library once and for all?
| await Requestor.PostStringAsync( | ||
| this.ApplyAllParameters(createOptions, classUrl, false), | ||
| this.SetupRequestOptions(requestOptions), | ||
| cancellationToken).ConfigureAwait(false)); |
There was a problem hiding this comment.
I would format the last line (ConfigureAwait) like so:
return Mapper<StripeFileLink>.MapFromJson(
await Requestor.PostStringAsync(
this.ApplyAllParameters(createOptions, classUrl, false),
this.SetupRequestOptions(requestOptions),
cancellationToken
).ConfigureAwait(false));
I like this JavaScript/jQuery style formatting practice. A quick glance shows where the function call ends imo
edit: fixed spacing
There was a problem hiding this comment.
@jaymedavis We now use StyleCop for the coding styles and one of the rules requires that the last bit is on the same line as cancelation token so it would have to look like what @ob-stripe did.
ob-stripe
force-pushed
the
ob-file-link
branch
3 times, most recently
from
b80f114 to
bc0f7af
Compare
|
I've reverted the sync/async thing for now. I still want to move forward with this, but this PR isn't the best place for it. |
|
ptal @remi-stripe |
| @@ -1,6 +1,7 @@ | |||
| namespace Stripe | |||
| { | |||
| using System; | |||
|
|
|||
There was a problem hiding this comment.
curious about that one, is it a convention you want to start respecting? I don't think it was in any of the StyleCop rules
There was a problem hiding this comment.
It doesn't seem to conventional in C#, so I've reverted.
| /// The ID of the file. | ||
| /// </summary> | ||
| [JsonProperty("file")] | ||
| public string File { get; set; } |
There was a problem hiding this comment.
The convention in stripe-dotnet is to name things like this with Id at the end.
There was a problem hiding this comment.
Nice catch. Fixed.
| public bool? Expired { get; set; } | ||
|
|
||
| [JsonProperty("file")] | ||
| public string File { get; set; } |
ob-stripe
force-pushed
the
ob-file-link
branch
2 times, most recently
from
98223b9 to
a97a3fa
Compare
r? @brandur-stripe @remi-stripe
cc @stripe/api-libraries
Adds support for file_links.