-
Notifications
You must be signed in to change notification settings - Fork 573
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
Better serialization #1478
Better serialization #1478
Conversation
3144510
to
416578d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me and I like how it was re-organized! As usual though this is core to the library and I'm less comfortable approving myself so please be careful and all that :)
6180511
to
357e31d
Compare
357e31d
to
74381f0
Compare
r? @brandur-stripe @remi-stripe |
74381f0
to
bd1d7b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, awesome work here OB! Love the clarity, and love how many new tests are coming in for these primitives :)
Didn't find anything objectionable in here. Maybe one thing I'd be a little careful about is getting to overzealous with extension methods — they're super cool, but there is a clarity trade-off in that it's immediately clear where StringUtils.ToSnakeCase
comes from, but not where str.ToSnakeCase
comes from without going up to check the imports.
It's not a big deal in C# because probably everyone is using an IDE of some sort anyway, but something to think about, especially in instances where the extension methods aren't expected to have a whole lot of use, so you don't really need the short-hand that badly anyway.
LGTM.
Great point, I did get a little carried away with the extension methods :) Looking at Newtonsoft.Json's source code, they do have a static method named exactly |
bd1d7b8
to
247a38f
Compare
Summary of changes:
ParameterBuilder
toFormEncoder
StringUtils
with one new method:ToSnakeCase()
: converts strings fromCamelCase
tosnake_case
. This is used forExpand*
properties on services, to convert from the service's property name (e.g.ExpandFooBar
) to the Stripe property name (e.g.foo_bar
).BaseOptions
:ToQueryString()
: returns a form-encoded query string containing all of the parameters in the options class, including the ones set in theExtraParams
dictionary and theExpand
list.Service
:Expansions()
: returns a list of strings with the properties to expand. E.g. if the service has three propertiesExpandFoo
,ExpandBar
andExpandAllTheThings
and the last two are set totrue
, the method will return a list containing"bar"
and"all_the_things"
.ApplyAllParameters()
onService
Services/
to eitherServices/_base
(for base classes meant to be derived from) orServices/_common
(for options classes that can be used as-is and that are not specific to a single service)Since we have pretty good test coverage for encoding (and I've added a few more tests) I'm fairly confident that I haven't broken anything, but there are a few changes:
expand[0]=foo&expand[1]=bar
). Previously, they were encoded without indices (expand[]=foo&expand[]=bar
). This was inconsistent with the way regular lists are encoded. Now all lists are encoded the same way (I've also checked that the server accepts this).I'm targeting a new
integration-v23
branch despite the fact that there are no breaking changes in this PR, since I have a few more updates planned. The breaking changes should be fairly limited this time around though!