-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add command line tool module #4
Conversation
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.
Can you please break down the PR to a few commits? Right now it's really hard to review the whole thing, as it spans lots of files and many lines of code. Just find packs of code that make sense to group together; even without separating code of a file to more commits, but simply separating files, will be helpful.
I've added a few comments so that they don't get lost after the regrouping.
TransifexNativeSDK/clitool/src/main/java/com/transifex/clitool/MainClass.java
Show resolved
Hide resolved
TransifexNativeSDK/clitool/src/main/java/com/transifex/clitool/StringXMLAdaptor.java
Outdated
Show resolved
Hide resolved
TransifexNativeSDK/clitool/src/main/java/com/transifex/clitool/StringXMLAdaptor.java
Outdated
Show resolved
Hide resolved
TransifexNativeSDK/clitool/src/main/java/com/transifex/clitool/StringXMLAdaptor.java
Outdated
Show resolved
Hide resolved
Created "common" module. Moved CDSHandler, including its unit tests, and LocaleData from "txsdk" to " common" module. Refactored classes that used it. CDSHanlder now uses pure Java methods. Android references have been removed or replaced by pure Java ones. A method has been added for pushing strings to CDS. Added TxPostData class to LocaleData. Removed example unit and intrumented tests from "txnative" and "app". Created CDSHanlderAndroid in "txsdk", which contains the async method that was originally part of CDSHandler.
Created StringXMlConverter to conver android XML string resources into LocaleData types.
MainClass contains the entry method class for the .jar file. Command line argument processing takes place here. Added push and clear commands. Updated readme with the "Transifex command line tool" section.
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.
Everything looks great! I haven't done any manual testing, as I don't have the time to set up everything right now. You could demo the behavior in a call.
Heads up: the UI of the CLI (text & colors) will need update. For iOS we did this after the end of the 3rd milestone, so we don't need to deal with this right now. (example: https://github.com/transifex/transifex-swift-cli/pull/3/files#diff-c06774f8b213e04abb3ac5a344d28388bcde8426d53cbf8482aca4fec5ea9958)
* | ||
* @return <code>true</code> if data were pushed successfully, <code>false</code> otherwise. | ||
*/ | ||
public boolean postSourceStrings(@NonNull LocaleData.TxPostData postData) { |
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.
Let's rename this to pushSourceString
, as it is in the other Native SDKs.
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.
pushSourceString
or pushSourceStrings
?
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.
Sorry, I meant pushSourceStrings
.
TransifexNativeSDK/common/src/test/java/com/transifex/common/CDSHandlerTest.java
Show resolved
Hide resolved
TransifexNativeSDK/clitool/src/test/java/com/transifex/clitool/StringXMLConverterTest.java
Show resolved
Hide resolved
CDSHandler has been updated so that the CDS's response is parsed to the corresponding LocaleData object. Affected classes and their unit tests have been updated. Improvements to test-only resources in txsdk The unit tests of txsdk require some resources. At the same time, we don't want these resources to exist when building the app or the android library. As a workaround we set these resources in the debug build variant. The downside is that the unit tests that run in release mode fail, because these resources are not accessible in this case. The updated txsdk gradle configuration adds the required resources only when running a test. The resources have been moved from the debug to the test directory. Some unit tests in the clitool module accessed these resourced by path and have now been updated to point to the new location. Improvements in transitive dependencies "clitool" module depends on "common" module, which depends on other libraries. In order to build a fat .jar that contains all needed libraries, we had to re-declare "common's libraries in "clitool". Now, this is not needed as we use the "runtimeClasspath" when building the fat .jar.
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.
Looks great!
Created a clitool module that builds a fat .jar file that can be executed.
Classes that are needed by txsdk and clitool have been moved into the new
pure java module common.
Updated readme with instructions about building and running the cli tool.