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

Update kotlin and other libraries and fix the code #982

Closed
wants to merge 4 commits into from

Conversation

planarvoid
Copy link
Contributor

In this PR I'm updating the core libraries and fixing code after the changes. I don't think there are any breaking changes except of what I fixed. I've run all the tests and tested the app as well and I think it works well. Let me know if You'd like me to split the PR into multiple PRs.

Test

  1. Make sure the app works as expected.

Review

@danilo04

Make sure strings will be translated:

  • If there are new strings that have to be translated, I have added them to the client's strings.xml as a part of the integration PR.

@planarvoid planarvoid requested a review from danilo04 May 5, 2022 11:38
@planarvoid planarvoid self-assigned this May 5, 2022
@danilo04
Copy link
Contributor

danilo04 commented Jun 6, 2022

Hi @planarvoid, thanks for the changes.

I think it can be a little risky upgrading to Android 31 since we are not sure other projects that depend on Aztec (e.g. mobile Gutenberg) will play well with the change. Let me know what do you think?

@ParaskP7
Copy link
Contributor

👋 @planarvoid @danilo04 !

Thank you both on working on this PR.

I think it can be a little risky upgrading to Android 31 since we are not sure other projects that depend on Aztec (e.g. mobile Gutenberg) will play well with the change. Let me know what do you think?

FYI: @AjeshRPai and @fluiddot recently helped with the targetSdkVersion = 31 upgrade (see here).

I am not sure why compileSdkVersion wasn't upgraded to 31 as well (currently 28). 🤔 If @AjeshRPai and @fluiddot is okay with that too, I am voting in favor of this upgrade, maybe as a separate upgrade to all other changes, so that it becomes easier for @AjeshRPai and @fluiddot to review and test the changes. 👍

PS: Also, I think now is a good time to extract all those references of 28 to an ext variable, just like it is already done for commonMinSdkVersion and commonTargetSdkVersion.

@fluiddot
Copy link
Contributor

I think it can be a little risky upgrading to Android 31 since we are not sure other projects that depend on Aztec (e.g. mobile Gutenberg) will play well with the change. Let me know what do you think?

@danilo04 good point, I'd like to note that we already upgraded compile and target sdk version to Android API 31 in Gutenberg (PR reference). Hence, I don't foresee potential conflicts due to this change. In any case, I'd advocate to do a double-check just in case.

I am not sure why compileSdkVersion wasn't upgraded to 31 as well (currently 28). 🤔

@ParaskP7 good point, I'm not sure either why the compileSdkVersion wasn't upgraded 🤔, maybe it wasn't necessary for the purpose of upgrading Android 12?

If @AjeshRPai and @fluiddot is okay with that too, I am voting in favor of this upgrade, maybe as a separate upgrade to all other changes, so that it becomes easier for @AjeshRPai and @fluiddot to review and test the changes. 👍

@planarvoid Yep, I'm also voting in favor of the upgrade. Let me know when the PR is ready to be tested, and I'll spare some time to test it in Gutenberg to confirm there's no breakage with the changes.

@danilo04
Copy link
Contributor

Hi @ParaskP7 @fluiddot. Good to know Gutenberg upgraded to Android 31 🎉 ! I think we just have to resolve the conflicts and also generate a new release build from trunk since we have some other changes merged, so @fluiddot can test those changes as well.

@planarvoid let me know if you have time to make the changes.

@ParaskP7
Copy link
Contributor

👋 @danilo04 @fluiddot !

@danilo04 good point, I'd like to note that we already upgraded compile and target sdk version to Android API 31 in Gutenberg (WordPress/gutenberg#44610). Hence, I don't foresee potential conflicts due to this change. In any case, I'd advocate to do a double-check just in case.

🥇

@ParaskP7 good point, I'm not sure either why the compileSdkVersion wasn't upgraded 🤔, maybe it wasn't necessary for the purpose of upgrading Android 12?

Yeap, I am not sure as well, maybe it was an oversight. 🤔 Cc @AjeshRPai

PS: To my knowledge and although it is (kind of) okay to have compileSdkVersion being of a lesser value than that of targetSdkVersion, it is (highly) recommended that compileSdkVersion is equal or higher than targetSdkVersion.

@planarvoid Yep, I'm also voting in favor of the upgrade. Let me know when the PR is ready to be tested, and I'll spare some time to test it in Gutenberg to confirm there's no breakage with the changes.

🥇

I think we just have to resolve the conflicts and also generate a new release build from trunk since we have some other changes merged, so @fluiddot can test those changes as well.

Since this PR is also about Kotlin, I would recommend to do the compileSdkVersion = 31 upgrade on a separate PR and then continue with this PR (if need be). Wdyt? 🤔 Cc @danilo04 @planarvoid

@AjeshRPai
Copy link
Contributor

Yeap, I am not sure as well, maybe it was an oversight.

Yep, it was. During the migration, I missed this. My Bad.

PS: To my knowledge and although it is (kind of) okay to have compileSdkVersion being of a lesser value than that of targetSdkVersion, it is (highly) recommended that compileSdkVersion is equal or higher than targetSdkVersion.

Yeah, I completely agree.

PS: Also, I think now is a good time to extract all those references of 28 to an ext variable, just like it is already done for commonMinSdkVersion and commonTargetSdkVersion.

I also vote for this suggestion as it would make it easier in the future to manage the versions.

@ParaskP7
Copy link
Contributor

👋 @planarvoid !

I guess this PR can be now closed and we could (potentially) create another separate PR to update compileSdkVersion to 31, right? 🤔

Are there any other major changes in this PR that weren't already included within these below PRs already, that is, apart from the compileSdkVersion and other dependency updates I am seeing in the diff?

@ParaskP7
Copy link
Contributor

ParaskP7 commented Dec 6, 2022

👋 @planarvoid @danilo04 !

As per my comment above, let's consider closing this PR for now. Wdyt? 🤔

@danilo04
Copy link
Contributor

We agree @ParaskP7. Closing the PR.

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

Successfully merging this pull request may close these issues.

None yet

5 participants