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

Support categories #152

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ilabsvictor
Copy link
Contributor

No description provided.

Copy link

@eb-smith-affirm eb-smith-affirm left a comment

Choose a reason for hiding this comment

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

One suggestion. Thanks for adding HelpDoc.

unitPrice:(NSDecimalNumber *)unitPrice
quantity:(NSInteger)quantity
URL:(NSURL *)URL
categories:(nullable NSArray<NSArray<NSString *> *> *)categories

Choose a reason for hiding this comment

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

I'm coming into this cold and don't have context. But an array of arrays is kind of weird to wrap one's head around.

Would it be clearer to define an AffirmCategory like:

@interface AffirmCategory
@property (nonatomic, nonnull) NSString name;
@property (nonatomic, nullable) NSArray<NSString*> subCategories;
... etc.

and have categories be an NSArray<AffirmCategory*>*?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now, both the backend and Android SDK use a two-dimensional string array without any subdivisions. Do we need to standardize this? If we decide to define an AffirmCategory, it only needs to include an array, the name isn't really necessary.

Backend:
https://docs.affirm.com/developers/reference/the-item-object

Android SDK:
https://github.com/Affirm/affirm-merchant-sdk-android/pull/137/files#diff-0f9e2c64c69d37cfd2646df60aecfb5bf5ce6eef5cbee13c2981134d38b47a0eR54

Copy link
Contributor

Choose a reason for hiding this comment

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

Due to some issues with GitHub Actions on the latest macOS, downgrading to macOS-12 can resolve the problem. You can find more details here: actions/runner-images#9591.

Choose a reason for hiding this comment

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

Ed's comment seems fine to me, might help with clarity.
Will we run into any issues in the future with this macOS "downgrade"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'll optimize it. The downgrade does not impact the SDK, it simply enables tests to run normally.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eb-smith-affirm @bf-affirm It has been optimized, thanks for your response.

@eb-smith-affirm
Copy link

Also the tests fail. What do you use as the CI/CD?

Copy link

@eb-smith-affirm eb-smith-affirm left a comment

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.

4 participants