Uh oh!
There was an error while loading. Please reload this page.
Update cards and addresses - #155
Conversation
waldyrious
left a comment
There was a problem hiding this comment.
This update seems correct from what I can tell, but it's unnecessarily hard to review, due to the changes being lumped up in a single commit.
The reordering of the fields, the formatting adjustments, and the changes in actual content of the fields[1] should have been committed separately from the addition of the new fields.
[1] why are these needed, anyway? That wasn't clear from the diff nor was it explained in the PR's opening comment.
rplopes
commented
Sep 25, 2018
I generally agree with the goal of keeping diffs small between commits. However, since the fix here was to replace the existing example with an updated API output, it feels like there's not a lot we could split and still get atomic, correct commits. We could reorder the old example to match the new example's order, to help with diff'ing later, but that's about it. The content of the fields didn't necessarily have to change. It did just to keep consistent with e.g. the new rates, the new fields, and so on. Since these PRs update cards, transactions and so on, which are dependent on each other, it seemed like an improvement to have examples that make sense between these different entities. |
waldyrious
commented
Sep 25, 2018
Ok, thanks for clarifying. I understand that it's not always possible / reasonable to maximize commit atomicity; I just wanted to make sure that it was a considered decision rather than a general practice. |
Add
normalizedandsettings.protectedto cards. Fix addresses formatting.