-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[in_app_purchase] 0.3.0 Migration guide #4137
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
Conversation
|
Thanks for adding this guide! I'll block some time to review this this afternoon. Requesting review from @stuartmorgan as well, as you reviewed the initial migration to billing client v5 (let me know if you'd prefer I find a different secondary reviewer Stuart 🙂 ). |
gmackall
left a comment
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 mostly LGTM, but I want to hold off on approving till I can finish reading through the linked android documentations tomorrow.
packages/in_app_purchase/in_app_purchase_android/migration_guide.md
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_android/migration_guide.md
Outdated
Show resolved
Hide resolved
acf6186 to
d3cec0e
Compare
gmackall
left a comment
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!
|
@gmackall sorry for the inconvenience, but I updated the subscription use cases once more based on feedback in flutter/flutter#127844, as the previous examples missed a crucial detail in fetching subscription offer details. |
stuartmorgan-g
left a comment
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.
The content looks great! Other than converting to code excerpts, just a few minor questions.
packages/in_app_purchase/in_app_purchase_android/migration_guide.md
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_android/migration_guide.md
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_android/migration_guide.md
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_android/migration_guide.md
Outdated
Show resolved
Hide resolved
packages/in_app_purchase/in_app_purchase_android/migration_guide.md
Outdated
Show resolved
Hide resolved
stuartmorgan-g
left a comment
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.
(Oops, I meant that to be a Request Changes for PR status tracking.)
packages/in_app_purchase/in_app_purchase_android/migration_guide.md
Outdated
Show resolved
Hide resolved
gmackall
left a comment
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.
Re-adding my approval as this has gone through a couple of changes since my last review, but still LGTM!
I also was not aware of the existence of code-excerpts, but will make sure to recommend them moving forward
stuartmorgan-g
left a comment
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, thanks!
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
test-exempt: documentation Does it make sense to call this file something more specific? (To put it another way, what do we plan to do when we add a second or third migration guide?) |
If we need more migration guides we would probably put them all in the one file, with top-level sections for each major version, since we don't know what version people will be starting from. |
|
Overriding stale tree status. |
The API changes introduced in 0.3.0 turned out to be bigger and more complicated than anticipated. This migration guide aims to help developers upgrade to 0.3.0 by giving context to the changes and treating some exemplary use cases.
The need for a migration guide was brought to my attention in flutter/flutter#127844.
A follow-up PR will update the README in the app-facing package to align with the breaking changes introduced in 0.3.0, as the current one is out-of-date.
Fixes flutter/flutter#127844
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).