[Modules Restructuring] Restructure Networking Module #16752
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file and its inner properties seem to be duplicate of the root level 'gradle.properties-example' file and/or outdated.
The root level '.gitignore' file should be all that is needed from now on.
The inner 'build.gradle' file from the 'WordPressNetworking' module should be all that is needed from now on.
Everything within the 'WordPressNetworking' module ('build.gradle' +
'src') is now moved to the new root 'networking' module.
With this change the new 'networking' module is effectively and
completely disassociated from the 'WordPress-Networking-Android' repo.
URL: https://github.com/wordpress-mobile/WordPress-Networking-Android
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
4 tasks
Contributor
|
Found 1 violations: The PR caused the following dependency changes:+\--- project :libs:networking
+ \--- com.automattic:rest:1.0.8
-\--- project :libs:networking:WordPressNetworking
- \--- com.automattic:rest:1.0.8
Please review and act accordingly
|
Contributor
You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr16752-6f2c174.apk), or scanning this QR code:
|
Contributor
You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr16752-6f2c174.apk), or scanning this QR code:
|
oguzkocer
approved these changes
Jun 15, 2022
Contributor
Author
|
👋 @ashiagr ! Thank you reviewing and testing this PR! 🙇
Yes indeed, this might be the best way forward with that. I already added a task for that on the parent issue for this modules restructuring overall work (search for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Parent #16562
Fixes #16567
This PR restructures the
networkingmodule by disassociating the WordPress-Networking-Android repo and removing the extraWordPressNetworkinglevel from it. This effectively makesnetworkinga pure module, without any extra inner levels or unnecessary build files lying around (likesettings.gradle,.gitignore, etc).PS: @ashiagr I added you as the main reviewer, that is, in addition to the @wordpress-mobile/owl-team team itself, since I know you were working with Reader and Discover in the past, which will come handy for testing purposes. As such, I think you might be the best person to sign-off on that change for WPAndroid. 🥇
Question (❓): As per my licensing question on this other related PR of mine here and since I don't see any
LICENCEfile for this module, like I did for theanalyticsmode, which is pointing toMIT, I am not sure if we should include a licence for this module too or exclude the licensing information from theanalyticsand all other modules in general so that we start depending solely on the root levelLICENCEthat WPAndroid is using, which isGPLv2, wdyt? 🤔@oguzkocer related to your comments on this other related PR of mine here I am still waiting for us to align on both, the module naming convention and the README file. Then, I'll proceed and make the necessary update(s), if any, on all the PRs. You will see me progressing with my PRs anyway just so that I kepp move forward, but that doesn't mean anything, I can and will make the change(s) we end up agreeing on afterwards and before I merge any of my PRs so don't worry if you see me creating more PRs like that.
To test:
Reader->DISCOVERtab.ReaderDiscoverLogic.ktclass, line123and theWordPress.getRestClientUtilsV2(...)call.The above is just one example of how this change can tested so that we could quickly verify the overall correctness of this PR. However, feel free to smoke test other aspects as well, especially if you are aware any or all the other places where
networkingis being used to fetch data instead of FluxC.Regression Notes
N/A
See
To testsection above.N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.