Modernize StripeConfiguration #1485

ob-stripe · 2019-01-23T21:18:36Z

r? @remi-stripe
cc @stripe/api-libraries

Modernize StripeConfiguration:

all configuration values are now represented by properties rather than GetFoo() / SetFoo() methods
default values are exposed as read-only properties
all values have XML doc comments
HttpTimeSpan is renamed to HttpTimeout, becomes non-nullable, and defaults to 80 seconds (like our other libraries)
setting the API key via ConfigurationManager.AppSettings now works when targeting .NET Standard 2.0
moved /v1 prefix out of the base URLs and into each service's path
got rid of the Urls internal class

Breaking changes:

StripeApiVersion is renamed to ApiVersion, and is now read-only

The existing Get/Set methods map to the new property as appropriate. Existing methods are annotated with Obsolete which will generate warnings when used, with a message indicating what to use instead.

ob-stripe · 2019-01-23T21:20:11Z

src/Stripe.net/Stripe.net.csproj

@@ -50,10 +50,6 @@
    <PackageReference Include="System.Collections.Immutable" Version="1.5.0" />
  </ItemGroup>

-  <PropertyGroup Condition=" '$(TargetFramework)' == 'net45' ">
-    <DefineConstants>$(DefineConstants);NET45</DefineConstants>


Turns out this wasn't needed. Preprocessor constants are automatically defined for the target framework: https://docs.microsoft.com/en-us/dotnet/standard/frameworks#how-to-specify-target-frameworks.

remi-stripe

Okay this all looks fine to me, but I have some comments (likely me misunderstanding some of it). Usual "I'm wary of reviewing such core changes, but not sure you have an alternative today)

remi-stripe · 2019-01-24T05:56:04Z

readme.md

@@ -35,7 +35,7 @@ You can configure the Stripe.net package to use your secret API key in one of tw
 a) In your application initialization, set your API key (only once once during startup):

 ```csharp
-StripeConfiguration.SetApiKey("[your api key here]");
+StripeConfiguration.ApiKey = "[your api key here]";


Does this remove SetApiKey entirely? Isn't this a breaking change? Or are we just making this cleaner but still supporting both (flagging now since it's the first thing in the review)

My intention was to avoid a breaking change by keeping the old methods around, but marking them as Obsolete to encourage users to upgrade to the new way of setting these values, and actually remove the methods in a future major version. This would make the upgrade a bit less painful for users.

I don't feel too strongly about it, if you'd rather we drop the old methods immediately that's fine by me.

Okay I think that's fine and a nicer experience for sure

src/Stripe.net/Infrastructure/Public/StripeConfiguration.cs

remi-stripe · 2019-01-24T06:00:47Z

src/Stripe.net/Infrastructure/Public/StripeConfiguration.cs

+        /// <remarks>
+        /// You can also set the API key using the <c>StripeApiKey</c> key in
+        /// <see cref="System.Configuration.ConfigurationManager.AppSettings"/>.
+        /// </remarks>


I feel weird about preprocessor commands + comments. Isn't it better to document it globally?

The issue is that we can't link to System.Configuration.ConfigurationManager.AppSettings with .NET Standard 1.2 because the class doesn't exist. I could just remove the link or the comment entirely, but it's kind of a shame.

Ah yeah I see what you mean. Ugh. Sounds good then!

src/Stripe.net/Infrastructure/Public/StripeConfiguration.cs

remi-stripe · 2019-01-24T06:02:34Z

src/Stripe.net/Infrastructure/Public/StripeConfiguration.cs

+        /// <see cref="HttpTimeout"/> property instead.
+        /// </summary>
+        // TODO: remove this property in a future major version
+        [Obsolete("Use StripeConfiguration.HttpTimeout instead.")]


We're already merging in a major version. Why not just do it?

remi-stripe · 2019-01-24T06:03:27Z

src/Stripe.net/Infrastructure/Public/StripeConfiguration.cs

        }

+        // TODO: remove everything below this in a future major version


Same as above, you're already planning version 23, why not do it?

remi-stripe · 2019-01-24T06:04:57Z

src/Stripe.net/Infrastructure/Requestor.cs

@@ -120,7 +117,7 @@ private static async Task<StripeResponse> ExecuteRequestAsync(HttpRequestMessage

        internal static HttpRequestMessage GetRequestMessage(string url, HttpMethod method, RequestOptions requestOptions)
        {
-            requestOptions.ApiKey = requestOptions.ApiKey ?? StripeConfiguration.GetApiKey();
+            requestOptions.ApiKey = requestOptions.ApiKey ?? StripeConfiguration.ApiKey;


Okay minor but need to flag: there's no thread safety and such, which I think is okay, but removing a getter prevents from ever adding this right? Though I guess it still has a getter/setter associated with it?

I'm not sure I follow. The only thing that changed with regards to the global API key is that StripeConfiguration.GetApiKey() is now StripeConfiguration.ApiKey and StripeConfiguration.SetApiKey(newKey) is now Stripe.Configuration.ApiKey = newKey.

Changing the global API key was never thread-safe. Users that want to use different API keys in a multi-threaded environment must set the API key per request via RequestOptions.

src/Stripe.net/Services/Account/AccountService.cs

ob-stripe · 2019-01-24T13:54:41Z

ptal @remi-stripe

remi-stripe · 2019-01-24T16:06:14Z

readme.md

@@ -35,7 +35,7 @@ You can configure the Stripe.net package to use your secret API key in one of tw
 a) In your application initialization, set your API key (only once once during startup):

 ```csharp
-StripeConfiguration.SetApiKey("[your api key here]");
+StripeConfiguration.ApiKey = "[your api key here]";


Okay I think that's fine and a nicer experience for sure

remi-stripe · 2019-01-24T16:08:03Z

src/Stripe.net/Infrastructure/Public/StripeConfiguration.cs

+        /// <remarks>
+        /// You can also set the API key using the <c>StripeApiKey</c> key in
+        /// <see cref="System.Configuration.ConfigurationManager.AppSettings"/>.
+        /// </remarks>


Ah yeah I see what you mean. Ugh. Sounds good then!

ob-stripe assigned remi-stripe Jan 23, 2019

ob-stripe commented Jan 23, 2019

View reviewed changes

ob-stripe mentioned this pull request Jan 23, 2019

stripe-dotnet v27.0.0 #1482

Merged

52 tasks

remi-stripe reviewed Jan 24, 2019

View reviewed changes

remi-stripe assigned ob-stripe and unassigned remi-stripe Jan 24, 2019

ob-stripe force-pushed the ob-stripe-configuration branch from cbc8b5a to 2e45d6a Compare January 24, 2019 10:09

Modernize StripeConfiguration

204f0f1

ob-stripe force-pushed the ob-stripe-configuration branch from 2e45d6a to 204f0f1 Compare January 24, 2019 13:52

ob-stripe added the breaking-api-change label Jan 24, 2019

ob-stripe assigned remi-stripe and unassigned ob-stripe Jan 24, 2019

remi-stripe approved these changes Jan 24, 2019

View reviewed changes

ob-stripe assigned ob-stripe and unassigned remi-stripe Jan 24, 2019

ob-stripe added the approved label Jan 24, 2019

ob-stripe merged commit b90a4e9 into integration-v23 Jan 24, 2019

ob-stripe deleted the ob-stripe-configuration branch January 24, 2019 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize StripeConfiguration #1485

Modernize StripeConfiguration #1485

ob-stripe commented Jan 23, 2019 •

edited

Loading

ob-stripe Jan 23, 2019

remi-stripe left a comment

remi-stripe Jan 24, 2019

ob-stripe Jan 24, 2019

remi-stripe Jan 24, 2019

remi-stripe Jan 24, 2019

ob-stripe Jan 24, 2019

remi-stripe Jan 24, 2019

remi-stripe Jan 24, 2019

ob-stripe Jan 24, 2019

remi-stripe Jan 24, 2019

ob-stripe Jan 24, 2019

remi-stripe Jan 24, 2019

ob-stripe Jan 24, 2019 •

edited

Loading

ob-stripe commented Jan 24, 2019

remi-stripe Jan 24, 2019

remi-stripe Jan 24, 2019

		}

		// TODO: remove everything below this in a future major version

Modernize StripeConfiguration #1485

Modernize StripeConfiguration #1485

Conversation

ob-stripe commented Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

remi-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

ob-stripe commented Jan 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe commented Jan 23, 2019 •

edited

Loading

ob-stripe Jan 24, 2019 •

edited

Loading