Make all value types in options classes nullable #1298
Conversation
ob-stripe
force-pushed
the
ob-nullable-value-types
branch
from
330bfaa to
3f8f512
Compare
| @@ -8,10 +8,10 @@ public class CardholderListOptions : ListOptions | |||
| public DateFilter Created { get; set; } | |||
|
|
|||
| [JsonProperty("email")] | |||
| public int Email { get; set; } | |||
| public string Email { get; set; } | |||
There was a problem hiding this comment.
😱 So many errors in those PRs I submitted :(
| BoolNullable = true, | ||
| }, | ||
| want = "?bool=False&bool_nullable=True&decimal=0&enum=test_one&int=0" | ||
| want = "?bool=True" |
There was a problem hiding this comment.
Those tests now make a lot more sense, yay!
| public class NullableValueTypes | ||
| { | ||
| [Fact] | ||
| public void EnsureAllValueTypesAreNullable() |
There was a problem hiding this comment.
Damn that is a really neat test. Reflection is really cool/powerful!
| List<string> results = new List<string>(); | ||
|
|
||
| // Get all classes that implement INestedOptions | ||
| var type = typeof(INestedOptions); |
There was a problem hiding this comment.
Shouldn't this be BaseOptions too?
There was a problem hiding this comment.
BaseOptions actually implements INestedOptions -- I changed this in #1276. We should probably rename INestedOptions to e.g. IOptions since it now simply indicates that a class contains request parameters, regardless of whether the parameters are nested or not.
There was a problem hiding this comment.
Oh I did miss that. Renaming would definitely make sense though I wonder if having an Interface for nested options is a good way to separate top-level options versus others
| // Sort results alphabetically | ||
| results = results.OrderBy(i => i).ToList(); | ||
|
|
||
| // Display our own error message (because Assert.Empty sucks) |
There was a problem hiding this comment.
You might want to remove (or reword) that last bit :)
| Console.WriteLine($"- {item}"); | ||
| } | ||
|
|
||
| Console.ResetColor(); |
There was a problem hiding this comment.
That's a lot of trouble with coloration and such instead of just dumping a list of properties. No strong feels though and I'm sure it looks neat, but wondering if it's too much work/how supported it is
ob-stripe
force-pushed
the
ob-nullable-value-types
branch
from
3f8f512 to
4e5486f
Compare
|
Made the requested changes, ptal @remi-stripe |
r? @remi-stripe
cc @stripe/api-libraries
Add a test to ensure that all value types in options classes are nullable.
(The test only runs under .NET Core 1.1 and not .NET Framework because reflection works differently, but that's okay: this is a "wholesome" test that enforces a code constraint, not a typical test that verifies the library works as expected under all runtimes.)
Actually make all value types in options classes nullable.