-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 target and compile sdk version to v29 #12947
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
05f0634
to
ce5459c
Compare
You can test the changes on this Pull Request by downloading the APK here. |
Generated by 🚫 dangerJS |
The lint error looks legitimate to me:
I left a comment on the gutenberg PR about this. |
@@ -29,4 +29,7 @@ private inline fun <reified T : Enum<T>> Intent.putEnumExtra(key: String, victim | |||
private inline fun <reified T : Enum<T>> Intent.getEnumExtra(key: String, default: T): T = | |||
getIntExtra(key, -1) | |||
.takeUnless { it == -1 } | |||
?.let { T::class.java.enumConstants[it] } ?: default | |||
?.let { | |||
@Suppress("UNNECESSARY_SAFE_CALL") |
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 @Suppress
needed?
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.
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.
Interestingly Android Studio doesn't seem to give me any warning if I remove the @Suppress
line like you did in the screenshot. 🤔
Certainly makes sense to include the @Suppress
since you're seeing the warning though. 👍
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.
Tested a number of different flows and everything looks good to me.
The mentioned PR has been closed (not merged). A fix is included in wordpress-mobile/gutenberg-mobile#2662. We'll need to wait until this new PR gets merged. |
@mchowning I've updated the gutenberg-mobile submodule and merged develop into this branch. I've smoke tested the editor + some other parts of the app and all seems to work as expected. Can I just merge the PR when CI finishes or do you want to check the changes before the merge? Thank you so much for all your help!!!!! 🙇 🙇 🙇 |
This PR updates compile and target sdk version to v29. I checked official android docs, but I haven't found any breaking change which would affect our app - privacy changes and behavior changes.
Not ready for merge - related prs in gutenberg repos need to be merged first and the ref needs to be updated
I think the lint is still failing since the changes in gutenberg repo are in a fork - when I targeted the submodule to the fork locally the lint passed without issues.
To test:
Test the app - anything you can think of. Ideally spent at least 10 minutes testing various flows.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.