Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[in_app_purchase] Add currencySymbol to ProductDetails #4115

Merged
merged 6 commits into from
Jul 8, 2021
Merged

[in_app_purchase] Add currencySymbol to ProductDetails #4115

merged 6 commits into from
Jul 8, 2021

Conversation

renefloor
Copy link
Contributor

Adds currencySymbol to ProductDetails.

With PR #4114 both Android and iOS have a currency symbol. This PR adds the currency symbol to the platform interface so it can be exposed in the app facing package.

Part of flutter/flutter#83984

This is a breaking change because it has a new required parameter in the constructor. I could make it optional, but that would be suboptimal for usage. I'm not sure if there is a migration guid needed, but it is stated in the changelog what is changed.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@renefloor renefloor requested review from cyanglaz and LHLL as code owners June 29, 2021 13:36
@google-cla google-cla bot added the cla: yes label Jun 29, 2021
@renefloor renefloor requested a review from mvanbeusekom June 29, 2021 13:36
BeMacized
BeMacized approved these changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@BeMacized BeMacized left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@renefloor
Copy link
Contributor Author

@stuartmorgan The publishable task fails because it is a breaking change and that is strongly discouraged.
I could make this not a breaking change by making the property optional, but that's not what the user of the package is expecting and this interface is only being used by our 2 platform implementations.

@stuartmorgan-g
Copy link
Contributor

@stuartmorgan The publishable task fails because it is a breaking change and that is strongly discouraged.

Yes, that's flutter/flutter#85391

I could make this not a breaking change by making the property optional, but that's not what the user of the package is expecting

Can you elaborate on what that means? If you're just referring to the type of the accessor you could mask that by defaulting it to '', and then ensuring that we always fill it in. Not ideal, but it avoids a breaking change throughout the package just to make a single type non-nullable.

and this interface is only being used by our 2 platform implementations.

This is inconsistent with the statement above. Either this class ends up as part of the public interface of the app-facing packages, in which case this is by definition not true because literally anyone could be using this interface, or it's not, in which case the details of the class are not relevant to clients.

@renefloor
Copy link
Contributor Author

If you're just referring to the type of the accessor you could mask that by defaulting it to '', and then ensuring that we always fill it in.

Indeed, I meant that it is required in the constructor, but by adding a default it is not required. I'll make it optional that way.

Either this class ends up as part of the public interface of the app-facing packages, in which case this is by definition not true because literally anyone could be using this interface, or it's not, in which case the details of the class are not relevant to clients.

In the app you (normally) only read the ProductDetails objects and you don't create them, so for reading them it is not breaking. But indeed it is possible to create them as well, so it would be a breaking change for the app facing packages.

@renefloor renefloor added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jul 5, 2021
@fluttergithubbot
Copy link

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite build_all_plugins_apk has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jul 5, 2021
@renefloor renefloor mentioned this pull request Jul 8, 2021
Merged
11 tasks
mvanbeusekom
mvanbeusekom approved these changes Jul 8, 2021
View reviewed changes
Copy link
Contributor

@mvanbeusekom mvanbeusekom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mvanbeusekom mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jul 8, 2021
@fluttergithubbot fluttergithubbot merged commit e1b4ba4 into flutter:master Jul 8, 2021
@renefloor renefloor deleted the iap/platform-currency-symbol branch July 8, 2021 13:43
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 8, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2021
@engine-flutter-autoroll
e1b4ba4 [in_app_purchase] Add currencySymbol to ProductDetails (flutt…
63567a3 
…er/plugins#4115)
@renefloor renefloor mentioned this pull request Jul 8, 2021
Merged
11 tasks
fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Jul 8, 2021
@engine-flutter-autoroll
e1b4ba4 [in_app_purchase] Add currencySymbol to ProductDetails (flutt…
a28ff4b 
…er/plugins#4115) (#86098)
amantoux pushed a commit to amantoux/plugins that referenced this pull request Jul 10, 2021
@renefloor @amantoux
[in_app_purchase] Add currencySymbol to ProductDetails (flutter#4115)
4b11494
Ralph-Li added a commit to Insight-Timer/plugins that referenced this pull request Jul 15, 2021
@Ralph-Li
Merge branch 'upstream_master'
567ee66 
* upstream_master: (40 commits)
  [image_picker] Image picker fix camera device (flutter#3898)
  [flutter_plugin_tools] Improve license-check output (flutter#4154)
  [webview_flutter] Fix broken keyboard issue link (flutter#3266)
  [flutter_plugin_tools] Support format on Windows (flutter#4150)
  [flutter_plugin_tools] Make unit tests pass on Windows (flutter#4149)
  [image_picker_for_web] Migrate image_picker to package:cross_file (flutter#4083)
  [various] Prepare plugin repo for binding API improvements (flutter#4148)
  [quick_actions] Add const constructor (flutter#4131)
  [in_app_purchase] Add iOS currency symbol to ProductDetails (flutter#4144)
  [in_app_purchase] Added priceCurrencySymbol to SkuDetailsWrapper (flutter#4114)
  [image_picker_platform_interface] Add methods that return package:cross_file (flutter#4072)
  [flutter_plugin_tools] Improve and test 'format' (flutter#4145)
  [flutter_plugin_tools] Only check target packages in analyze (flutter#4146)
  [in_app_purchase] Fix crash when retrieveReceiptWithError gives an error. (flutter#4138)
  [video_player] Pause video when it completes (flutter#3727)
  [in_app_purchase] Add currencySymbol to ProductDetails (flutter#4115)
  [in_app_purchase] Add documentation for price change confirmations (flutter#4092)
  [camera] android-rework part 8: Supporting modules for final implementation (flutter#4054)
  [plugin_platform_interface] Fix README broken link (flutter#4143)
  [various] Prepare plugin repo for binding API improvements (flutter#4137)
  ...
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
@renefloor @fotiDim
[in_app_purchase] Add currencySymbol to ProductDetails (flutter#4115)
0da7eb1
@darshankawar darshankawar mentioned this pull request Apr 30, 2024
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BeMacized BeMacized BeMacized approved these changes

@mvanbeusekom mvanbeusekom mvanbeusekom approved these changes

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

@LHLL LHLL Awaiting requested review from LHLL

Assignees
No one assigned
Labels
cla: yes p: in_app_purchase waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@renefloor @stuartmorgan-g @fluttergithubbot @BeMacized @mvanbeusekom