-
Notifications
You must be signed in to change notification settings - Fork 1
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
Upgrade gradle and project dependencies #10
Conversation
This commit also switches to maven-publish since bintray is not a viable option anymore. Note: metadata is left commented out in a few places because this information will be useful in later commits that specifically address publication requirements. The primary focus for now is to update / upgrade the APIs used in this implementation. It can be tested with other projects via `mavenLocal()`.
WordPressLint/src/main/java/org/wordpress/android/lint/WordPressIssueRegistry.kt
Show resolved
Hide resolved
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.
👋 @mkevins !
I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟
I have left a few suggestions (💡), one question (❓) and one minor (🔍) comment for you to consider. I am NOT going to approve this PR as I think it is better for us to at least discuss those before merging. I think it will be beneficial for us get this repo to that state where it is consistent with the rest of such library repos of ours, at least Gradle, AGP and build configuration wise.
EXTRAS
Suggestion (💡): I understand that you wanted to get this work done as quick as possible and thus not wanting to deal with it as per how you would have done normal development. But, I still recommend going for a multiple commit approach, followed by an adequate commit title/description, per change. This would have at least made it easy for me to review your changes. Having one big commit that does it all has personally slowed me down during my review process.
WordPressLint/src/main/java/org/wordpress/android/lint/WordPressIssueRegistry.kt
Show resolved
Hide resolved
...sLint/src/test/java/org/wordpress/android/lint/WordPressAndroidApiInViewModelDetectorTest.kt
Outdated
Show resolved
Hide resolved
WordPressLint/src/main/java/org/wordpress/android/lint/WordPressIssueRegistry.kt
Show resolved
Hide resolved
Thanks for all the feedback Petros! I've implemented your suggestions. My apologies about the "big" commit in this branch. My intentions were primarily with testing the dependent PR first, and I should have made that more clear, since that was an exploratory endeavor (i.e. any continued work on this would not be of much value if the rules were non-trivial to get working). However, since the rules are seemingly working as intended, I think it makes sense to proceed with further development of this repo (including getting it into good shape as your suggestions are very helpful with). This is ready for another pass when you have a moment. Thanks! |
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.
Many thanks for applying my suggestions @mkevins , approved, you rock! 🙇 ❤️ 🚀
I reviewed and tested this update, then resolved almost all comment. I just left 2 of them unresolved still, let me know if we can quickly get them done and merge this PR right after:
This command needs to be run twice.
Thanks for rereviewing. I've addressed the remaining suggestions 👍 . Ready for another pass 😄 |
👋 @mkevins !
Thanks for addressing everything, LGTM, we can totally merge this bad boy now! 🙇 ❤️ 🚀 |
Thanks for reviewing and testing! |
Description
This PR migrates to newer versions of Gradle, lint, and Kotlin. Since bintray is not a viable option anymore, it also switches to maven-publish, but publishing details have been left out for now.
Note: metadata is left commented out in a few places because this information will be useful in later commits that specifically address publication requirements. The primary focus for now is to update / upgrade the APIs used in this implementation. It can be tested with other projects via
mavenLocal()
.To test:
Unit tests
Run the unit tests within this project.
Test with WordPress-Android
Publish this locally (i.e. to
~/.m2/
):./gradlew assemble test publishToMavenLocal
Update the version in
./build.gradle
and addmavenLocal()
, by applying the changes from this patch:Run lint on the WordPress-Android project:
Many errors should be reported for the rule:
AndroidImportsInViewModel
(including many false positives). This rule will be removed in a later PR, but this out-dated rule can provide a verification, for now, that the rules are being applied correctly.