Convert all options to have one class per level in the JSON params #1301
Conversation
|
@ob-stripe I did the majority of those. It's missing the |
| @@ -124,5 +94,27 @@ public class SourceCreateOptions : BaseOptions, ISupportMetadata | |||
| /// </summary> | |||
| [JsonProperty("usage")] | |||
| public string Usage { get; set; } | |||
|
|
|||
| /* | |||
| Below we group all Source type specific paramters | |||
There was a problem hiding this comment.
Not sure if this is the right approach. It seems clean enough but I was a little bit wary about having Create in options names, because Create implies this is the top-level class name (for me at least). Did not want to spend too long on it and wanted you to have a first look though
There was a problem hiding this comment.
Yeah, that's a good point. Ideally we should have a clear naming convention, but in practice the dependency graph is complex and makes it pretty hard to come up with something that's applicable everywhere. What you have here is reasonable IMO.
We should just declare a single |
|
Looks great so far! So much cleaner ❤️ |
remi-stripe
force-pushed
the
remi-fix-all-nested-options
branch
from
7fdd34f to
5e35978
Compare
remi-stripe
changed the title
|
@ob-stripe Okay I did all the changes we discussed. I also took this as an opportunity to add new lines to all Options files which should make you happy. Can you do a full review? |
There was a problem hiding this comment.
Some comments, but overall this is super rad. Awesome work @remi-stripe
| /// A filter on the list based on the object date field. | ||
| /// </summary> | ||
| [JsonProperty("date")] | ||
| public DateFilterOptions DateFilter { get; set; } |
There was a problem hiding this comment.
Rename to DateRange for consistency.
|
|
||
| /// <summary> | ||
| /// A filter on the list based on the object due_date field. | ||
| /// </summary> | ||
| [JsonProperty("due_date")] | ||
| public DateFilter DueDate { get; set; } | ||
| public DateFilterOptions DueDateFilter { get; set; } |
There was a problem hiding this comment.
Rename to DueDateRange for consistency.
| @@ -3,21 +3,18 @@ | |||
| using System; | |||
| using Newtonsoft.Json; | |||
|
|
|||
| public class DateFilter | |||
| public class DateFilterOptions : INestedOptions | |||
There was a problem hiding this comment.
I think DateRangeOptions would be a better name: filtering on an exact date is also a "date filter", but this class now only handles ranges. It would also make the class name consistent with the properties names.
This ensures that params/class structure matches the API. Also changed DateFilter to match the API and remove custom encoding and renamed it to DateRangeOptions
remi-stripe
force-pushed
the
remi-fix-all-nested-options
branch
from
5e35978 to
a518a7c
Compare
|
@ob-stripe Fixed, PTYAL |
|
LGTM! |
This PR takes all existing Options classes and splits the properties per level in the JSON. This is to ensure that our class structure matches our API naming as in other libraries. It also makes the code easier to re-use when hashes are shared across resources.
cc @stripe/api-libraries