-
Notifications
You must be signed in to change notification settings - Fork 367
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
Prettier codegen #827
Prettier codegen #827
Conversation
ffb0371
to
6fefa7c
Compare
$ function dodiff { git diff origin/master --ignore-all-space | grep '^[+-]' | grep -v "^---" | grep -v "^+++"; }
$ function percent_diff { echo "scale=3; $(dodiff | $1 | wc -l)/$(dodiff | wc -l)" | bc -l; }
$ percent_diff 'grep String\.format'
.028
$ percent_diff 'grep Builder.params'
0
$ percent_diff 'grep public.*Builder'
.310
$ percent_diff 'grep \.request'
.016 |
I think most of this is method ordering still, though. |
err, ordering of the embedded class definitions |
3401a48
to
07d0bb8
Compare
Seems like the biggest offenders are the *Params.java files:
|
61afc94
to
40fb30e
Compare
Ok, made the diff much smaller, adding some namespacing (and two whitespace changes which were most of the raw diff). Biggest offenders now:
|
f8db34b
to
9c1f9e3
Compare
src/main/java/com/stripe/model/BalanceTransactionSourceTypeAdapterFactory.java
Outdated
Show resolved
Hide resolved
9c1f9e3
to
3f7817f
Compare
3f7817f
to
05ec36e
Compare
839bedf
to
c06d4b4
Compare
c06d4b4
to
7843094
Compare
r? @ob-stripe |
Here's the list of all changes I noticed: ℹ️ = cosmetic change
Most of these seem peaceful to me. Can I just ask you to fix these two?
ptal @richardm-stripe |
ptal @ob-stripe please also approve https://github.com/stripe/stripe-api-codegen/pull/129 |
Looks great! One final nit: can you fix the missing whitespace in adapter factory classes? Like here: https://github.com/stripe/stripe-java/pull/827/files#diff-c235a01258aeff703a67c9fd41deff9dL63 |
@ob-stripe done |
@ob-stripe good to go now? |
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.
LGTM!
Aside from the explicit null check in nested resources that was removed, all the other changes are noted are cosmetic and should have no impact on the output JAR.
Let's run the compatibility checker script to double check that the prettier version is 100% compatible with the current one, but I think this is ready to ship!
Er, belay that, looks like tests are not passing. |
It's because of this change:
ptal @richardm-stripe |
There was one last change in b7b8e34
|
Released as 11.7.0 \o/ |
Result of https://github.com/stripe/stripe-api-codegen/pull/128