Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added explicit parameter name to checkTrialOrIntroDiscountEligibility for consistency #1362

Merged
merged 3 commits into from Mar 9, 2022
Merged

Added explicit parameter name to checkTrialOrIntroDiscountEligibility for consistency #1362

merged 3 commits into from Mar 9, 2022

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Mar 7, 2022 •
edited by jira bot

Fixes CF-287
Now we have checkTrialOrIntroDiscountEligibility(product:) and checkTrialOrIntroDiscountEligibility(productIdentifiers:)

@NachoSoto NachoSoto requested review from codykerns and a team March 7, 2022 19:22
@NachoSoto
Copy link
Contributor Author

This will require a minor version bump (4.1). Should we leave this un-merged for now?

@NachoSoto NachoSoto force-pushed the checkTrialOrIntroDiscountEligibility-consistency branch from 994fcac to f162613 Compare March 7, 2022 19:47
codykerns
codykerns approved these changes Mar 8, 2022
View reviewed changes
Copy link
Member

@codykerns codykerns left a comment

Choose a reason for hiding this comment

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

LGTM!

@aboedo
Copy link
Member

aboedo commented Mar 8, 2022

This will require a minor version bump (4.1). Should we leave this un-merged for now?

This would break compilation, right? If so it requires a major bump, minors are for new APIs. I realize that Xcode provides an auto-fix it, but I still think this could be a surprise to developers who upgrade to 4.1 without expecting compilation to break, or who have their CI set up to automatically update to the latest minor

@NachoSoto
Copy link
Contributor Author

Oh duh of course. I think there's 3 options then:

  • Leave this here and don't merge until we want to make a major version bump (not ideal).
  • Don't do anything and leave this inconsistency.
  • Add the old method without a parameter name, but mark it as deprecated. It would provide a warning but compilation would still work. I'd vote for this.

@aboedo
Copy link
Member

aboedo commented Mar 8, 2022

yeah a deprecation with an auto-fix it seems solid to me 👍

@NachoSoto NachoSoto force-pushed the checkTrialOrIntroDiscountEligibility-consistency branch from f162613 to 2b9f450 Compare March 8, 2022 17:12
NachoSoto
NachoSoto commented Mar 8, 2022
View reviewed changes
Purchases/Misc/Deprecations.swift
Comment on lines +10 to +17
// Deprecations.swift
//
// Created by Nacho Soto on 3/8/22.

import Foundation
import StoreKit

// swiftlint:disable line_length missing_docs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created this similarly to Obsoletions.swift where we can put all these.
I think the swiftlint:disable helps, we could have probably done the same in Obsoletions.swift. Easier to read with long lines instead of breaking every platform, and we don't actually want to document these. What do you all think?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, good call 👍

@NachoSoto NachoSoto force-pushed the checkTrialOrIntroDiscountEligibility-consistency branch from 2b9f450 to 881e8fd Compare March 8, 2022 17:39
aboedo
aboedo approved these changes Mar 8, 2022
View reviewed changes
Purchases/Misc/Deprecations.swift
Comment on lines +10 to +17
// Deprecations.swift
//
// Created by Nacho Soto on 3/8/22.

import Foundation
import StoreKit

// swiftlint:disable line_length missing_docs
Copy link
Member

Choose a reason for hiding this comment

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

yeah, good call 👍

Purchases/Misc/Deprecations.swift Show resolved Hide resolved
@NachoSoto NachoSoto force-pushed the checkTrialOrIntroDiscountEligibility-consistency branch from 881e8fd to db4cc3d Compare March 8, 2022 21:11
@NachoSoto
Added explicit parameter name to `checkTrialOrIntroDiscountEligibilit…
d85dca1 
…y` for consistency

Fixes [CF-287]
Now we have `checkTrialOrIntroDiscountEligibility(product:)` and `checkTrialOrIntroDiscountEligibility(productIdentifiers:)`
@NachoSoto
Added deprecations
b49477b
@NachoSoto
Updated Deprecations.md
8774bb7
@NachoSoto NachoSoto force-pushed the checkTrialOrIntroDiscountEligibility-consistency branch from dffa84a to 8774bb7 Compare March 9, 2022 21:11
@NachoSoto NachoSoto merged commit 17363f2 into main Mar 9, 2022
@NachoSoto NachoSoto deleted the checkTrialOrIntroDiscountEligibility-consistency branch March 9, 2022 21:12
@aboedo aboedo mentioned this pull request Mar 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aboedo aboedo aboedo approved these changes

@codykerns codykerns codykerns approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@NachoSoto @aboedo @codykerns