-
Notifications
You must be signed in to change notification settings - Fork 32
chore: proguard support #316
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
Conversation
| # Uncomment this to preserve the line number information for | ||
| # debugging stack traces. | ||
| -keepattributes SourceFile,LineNumberTable |
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.
by default this is stripped out from the apk
| } | ||
|
|
||
| // TODO: CLI path config | ||
| args.add("posthog-cli") |
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.
This will fail if posthog-cli is not installed. Is there a way to run a 'which posthog-cli' here and grab the path?
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.
which posthog-cli will only return a path if its within the $PATH anyway, and in this case, the command above wont fail.
this will require posthog-cli to be installed, the dif from Xcode is that this has access to the full $PATH env. var. differently than Xcode that is very limited
| dependencies { | ||
| compileOnly(gradleApi()) | ||
| // pinned to 8.0.x so we compile against the min. supported version. | ||
| compileOnly("com.android.tools.build:gradle:8.0.2") |
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.
Just checking, this means that users can use any AGP 8.x.x right, not only 8.0.x?
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.
Correct
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.
As long as the 8.x isnt breaking any public APIs we depend on.
| return field | ||
| } | ||
|
|
||
| if (!field.get().asFile.name.equals(name)) { |
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.
Not sure I completely get what this is doing exactly, but can't we do this in taskAction() proactively instead of the getter?
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.
this is called twice
Line 57 in 2cdc37b
| outputDir = InjectPostHogMetaPropertiesIntoAssetsTask::outputDir, |
so i'd have to duplicate or extract to a method, and then if we need a 3rd time i'd need to remember that i have to call a method before, this ensures that whenever we need to read this property, the getter does the job.
its not pretty but it means we dont have to remember this workaround nor duplicate code
ioannisj
left a comment
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.
Not sure I can do more in-depth technical review here. LG to me, plus most of this is tested code already so 👍
💡 Motivation and Context
💚 How did you test it?
📝 Checklist