-
Notifications
You must be signed in to change notification settings - Fork 54
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
Adds pricePerPeriod functions to PricingPhase #1789
Conversation
fun formattedPriceInMonths(locale: Locale = Locale.getDefault()): String { | ||
return price.pricePerMonth(billingPeriod, locale).formatted | ||
return pricePerMonth(locale).formatted |
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.
Should we deprecate formattedPriceInMonths()
and direct people to use pricePerMonth().formatted
directly? What do you think? It might be confusing that 2 methods do pretty much the same thing.
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.
Yeah, I agree with that. formattedPriceInMonths
doesn't really fit the naming we've been using 👍
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.
Alright, done: 5947b8b
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1789 +/- ##
==========================================
+ Coverage 82.87% 82.90% +0.02%
==========================================
Files 223 223
Lines 7673 7679 +6
Branches 1078 1078
==========================================
+ Hits 6359 6366 +7
+ Misses 893 892 -1
Partials 421 421 ☔ View full report in Codecov by Sentry. |
fun formattedPriceInMonths(locale: Locale = Locale.getDefault()): String { | ||
return price.pricePerMonth(billingPeriod, locale).formatted | ||
return pricePerMonth(locale).formatted |
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.
Yeah, I agree with that. formattedPriceInMonths
doesn't really fit the naming we've been using 👍
* @param locale Locale to use for formatting the price. Default is the system default locale. | ||
*/ | ||
@JvmOverloads | ||
fun pricePerWeek(locale: Locale = Locale.getDefault()): Price = |
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.
I'm surprised codecov says these are not tested when we are adding tests in this PR 🤔. Not sure what's happening though... Any ideas?
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.
That's very weird. I'm investigating. 🔍
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.
I figured it out! I wasn't testing the overloads without Locale
! 46b8d7a
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.
Ohhh makes sense. Thanks for debugging that!
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!
assertThat(actualYearlyNoLocale.amountMicros).isEqualTo(expected.yearly.amountMicros) | ||
assertThat(actualMonthlyFormatted).isEqualTo(expected.monthly.formatted) | ||
// Hard to test anything else if the locale is not deterministic, besides formatting exactly like the | ||
// implementation does. |
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.
I guess we could mock the Locale.getDefault
static method... But I don't think it's a big deal.
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.
Ah yes, true! I don't think it's super important either though, because this is deprecated, and the implementation it calls is fully tested.
**This is an automatic release.** ### New Features * Adds pricePerPeriod functions to PricingPhase (#1789) via JayShortway (@JayShortway) ### Dependency Updates * Bump rexml from 3.2.9 to 3.3.2 (#1788) via dependabot[bot] (@dependabot[bot]) * Bump fastlane from 2.221.1 to 2.222.0 (#1797) via dependabot[bot] (@dependabot[bot]) ### Other Changes * Adds the baseline file to the Detekt IntelliJ plugin settings. (#1798) via JayShortway (@JayShortway) * Adds the Detekt IntelliJ plugin settings to version control. (#1796) via JayShortway (@JayShortway) * Diagnostics: Remove unused anonymizer (#1795) via Toni Rico (@tonidero) Co-authored-by: revenuecat-ops <[email protected]>
As the title says.
Context:
formattedPricePerMonth
helper toStoreProduct
andPricingPhase
#1255