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
Update naming for AdClient #932
Conversation
Apple might have updated their app scanning to find any mention of iAd, rejecting apps even though it isn't used.
Update tests
IntegrationTests/XcodeDirectIntegration/XcodeDirectIntegration.xcodeproj/project.pbxproj
Show resolved
Hide resolved
@@ -29,7 +29,7 @@ NS_ASSUME_NONNULL_BEGIN | |||
|
|||
- (nullable NSString *)identifierForVendor; | |||
|
|||
- (void)adClientAttributionDetailsWithCompletionBlock:(RCAttributionDetailsBlock)completionHandler; | |||
- (void)afficheClientAttributionDetailsWithCompletionBlock:(RCAttributionDetailsBlock)completionHandler; |
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.
why affiche? affiche = attach in french? wondering how we can document this while avoiding the-words-that-must-not-be-named.
or maybe we could do a funny misspelling like AddClyent
?
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.
+1. That'd make it easier for devs to track down the original class. adKlient
?
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.
Lolll good idea
AddClyent
I kinda have insight into how Apple does scanning for words (I was part of the China Online Store launch). At least there, they look for permutations of the same word using different letters that have the same sound or are similar. So AdKlient
would get caught, @dClient
@dKlient
as well. The word needs to be different enough from the word they are scanning for.
This is all just a guess, of course.
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.
So actually, I'm pretty sure AddClyent
would also get caught.
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 left a comment in the other PR regarding linking to the original classes through a rev.cat link, that applies here as well
@@ -29,7 +29,7 @@ NS_ASSUME_NONNULL_BEGIN | |||
|
|||
- (nullable NSString *)identifierForVendor; | |||
|
|||
- (void)adClientAttributionDetailsWithCompletionBlock:(RCAttributionDetailsBlock)completionHandler; | |||
- (void)afficheClientAttributionDetailsWithCompletionBlock:(RCAttributionDetailsBlock)completionHandler; |
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.
+1. That'd make it easier for devs to track down the original class. adKlient
?
@@ -508,7 +508,7 @@ | |||
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; | |||
ASSETCATALOG_COMPILER_GLOBAL_ACCENT_COLOR_NAME = AccentColor; | |||
CLANG_ENABLE_MODULES = YES; | |||
CODE_SIGN_IDENTITY = "iPhone Developer"; | |||
CODE_SIGN_IDENTITY = "iPhone Distribution"; |
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.
Is this intended? I am kinda out of sync with the certs changes
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.
oops, clicked resolve when I meant to reply.
I think this maps the distribution cert to both debug and release, ideally we'd map debug -> development, release -> distribution, but it's not a big deal given that we don't run these directly, just for test archiving
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 don't think so, I backed this out.
This reverts commit 41c890e.
…ts with the others
Add comments with links to the classes we are potentially calling.
Apple might have updated their app scanning to find any mention of iAd, rejecting apps even though it isn't used.