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.

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

Extract CardAttributes from CardType #119

Open
wants to merge 22 commits into
base: next-major-version
Choose a base branch
from

Conversation

sshropshire
Copy link
Contributor

@sshropshire sshropshire commented Jan 24, 2023

Summary of changes

  • Make CardType a pure enum
  • Create CardAttributes to hold metadata about a particular card brand
  • Move additional card parsing logic into CardParser

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

@sshropshire sshropshire marked this pull request as ready for review January 25, 2023 17:56
@sshropshire sshropshire requested a review from a team as a code owner January 25, 2023 17:56
@@ -23,9 +24,9 @@ ext["signing.secretKeyRingFile"] = System.getenv('SIGNING_KEY_FILE') ?: ''

version '5.4.1-SNAPSHOT'
ext {
compileSdkVersion = 30
compileSdkVersion = 33
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a changelog for these changes?

@@ -1,2 +1 @@
android.enableJetifier=true
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with this one, is a changelog needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! I updated the CHANGELOG with the relevant entries.

* Character#isDefined(char)} is {@code false}).
* @see <a href="http://en.wikipedia.org/wiki/Luhn_algorithm">Luhn Algorithm (Wikipedia)</a>
*/
public static boolean isLuhnValid(String cardNumber) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is all going into a next-major-version branch is it worth explicitly calling out these breaking changes in the changelog? On iOS we have a breaking changes section where we've listed items like this. I realize it's a lot of functions to call out but since there isn't a docstring calling out it could be removed at any time, it may be helpful. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good callout. I'll add some CHANGELOG notes that it's no longer in the public facing API.

Comment on lines +160 to +164
val result = mutableMapOf<CardType, CardAttributes>()
for (item in items) {
result[item.cardType] = item
}
return result
Copy link
Contributor

Choose a reason for hiding this comment

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

Take it or leave it comment - if there's a clean/easy way to just create a map literal for knownCardBrandAttributes, might be nice not to need this additional helper function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. It's a compile time vs. runtime tradeoff. I initially went for a when statement but duplicating CardType felt like a code smell so I refactored to creating the map at runtime.

mapOf(
  CardType.VISA to CardAttributes(CardType.VISA, "^4\\d*", R.drawable.bt_ic_visa, 16, 16, 3, R.string.bt_cvv, null)
)

* Move all classes to `com.braintreepayments.api` package
* Remove Jetifier now that AndroidX is fully supported
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants